fix(reports): wait for ECharts paint before capturing report screenshots - #43077
Draft
eschutho wants to merge 3 commits into
Draft
fix(reports): wait for ECharts paint before capturing report screenshots#43077eschutho wants to merge 3 commits into
eschutho wants to merge 3 commits into
Conversation
Scheduled dashboard reports could deliver a screenshot with blank chart regions. The readiness gate treated a holder as "rendered" once .slice_container was present, but that mounts when data arrives -- before the ECharts canvas is drawn -- so the capture could fire on an empty canvas. Tag the ECharts host .echarts-host and add .echarts-render-finished only in the ECharts finished event, the single signal that the canvas is fully painted (chartStatus/onRenderSuccess both fire pre-paint). The readiness gate now treats a holder with an unpainted host as non-terminal (mounted_unpainted) and waits for it, failing loud on timeout instead of capturing blank. Applies to the dashboard gate and the single chart-container gate. Only ECharts hosts are gated: DOM/SVG vizzes paint on commit and non-ECharts canvas vizzes have no .echarts-host, so they are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Verifies the host carries echarts-host and that echarts-render-finished is set only when the ECharts finished event fires (not on mount/setOption). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43077 +/- ##
==========================================
+ Coverage 66.00% 66.77% +0.77%
==========================================
Files 2866 2862 -4
Lines 162263 161643 -620
Branches 37346 37283 -63
==========================================
+ Hits 107097 107933 +836
+ Misses 53100 51666 -1434
+ Partials 2066 2044 -22
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:
|
Import order, prettier formatting, and jest-dom toHaveClass. No logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scheduled dashboard reports (and the report screenshot path generally) can deliver an image with blank chart regions. The non-tiled readiness gate added in #42624 / #42253 treats a chart holder as rendered as soon as
.slice_containeris present with no.loading. But.slice_containermounts when the chart's data arrives — before the ECharts canvas is actually drawn — so afull_pagecapture can fire on an empty canvas.There is no existing "render finished" signal for screenshots (see the comment in
Echart.tsxthat disables animation for exactly this reason).chartStatus === 'rendered'/onRenderSuccessboth fire on loadable module-load, before paint. The only signal that guarantees the canvas is painted is ECharts' ownfinishedevent.Fix
plugin-chart-echarts/.../Echart.tsx): tag the canvas host with the runtime classecharts-host, and addecharts-render-finishedonly in the EChartsfinishedevent (cleared before eachsetOption). Classes are used because production stripsdata-test.screenshot_utils.py): a holder that still contains an unpainted.echarts-hostis non-terminal (newmounted_unpaintedstate) — the gate waits for it and fails loud on timeout rather than capturing blank. Applied to both the dashboard holder gate and the singlechart-containergate.Only ECharts hosts are gated. DOM/SVG vizzes paint synchronously on React commit, and non-ECharts canvas vizzes (deck.gl, mapbox, OpenLayers) have no
.echarts-host, so they are unaffected (and the gate never waits on them — no risk of hanging those reports). Giving those canvas vizzes their own completion markers is a natural follow-up.This is complementary to #42901 (which grows the viewport so off-screen holders mount): that ensures holders are present; this ensures they are painted before capture.
Testing
false; a painted host passes; DOM/non-ECharts holders are unchanged; off-screen holders are still skipped.finished-means-painted premise was verified directly against echarts 5.6 (thefinishedevent fires only once the canvas is fully drawn, with animation on or off).Additional info
🤖 Generated with Claude Code