fix(reports): wait for ECharts animation after spinners clear in non-tiled screenshots#41428
fix(reports): wait for ECharts animation after spinners clear in non-tiled screenshots#41428msyavuz wants to merge 1 commit into
Conversation
…tiled screenshots
dosubot
Bot
added
alert-reports
Code Review Agent Run #8949f7Actionable Suggestions - 0Review 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41428 +/- ##
==========================================
- Coverage 64.44% 64.43% -0.01%
==========================================
Files 2655 2655
Lines 145491 145515 +24
Branches 33585 33589 +4
==========================================
+ Hits 93762 93764 +2
- Misses 50028 50047 +19
- Partials 1701 1704 +3
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if selenium_animation_wait > 0: | ||
| page.wait_for_timeout(selenium_animation_wait * 1000) |
There was a problem hiding this comment.
Suggestion: This adds a second animation delay in the non-tiled flow, but the method already performs an unconditional SCREENSHOT_SELENIUM_ANIMATION_WAIT earlier before branching. With non-zero config values, screenshots now wait twice (effectively 2x the configured delay), which can significantly slow report generation and cause avoidable timeout pressure. Move the existing pre-branch wait to the post-spinner location (instead of adding a second wait), so the delay runs once at the correct time. [performance]
Severity Level: Major ⚠️
- ⚠️ Non-tiled Playwright PNG reports wait double animation time.
- ⚠️ Report generation slower, increased timeout pressure in jobs.Steps of Reproduction ✅
1. Trigger a screenshot via `Screenshot.get_screenshot()` in
`superset/utils/screenshots.py:212-218`, which instantiates a WebDriver (Playwright when
enabled) and calls `driver.get_screenshot(self.url, self.element, user)`.
2. In the Playwright implementation `WebDriverPlaywright.get_screenshot()` at
`superset/utils/webdriver.py:320-355`, the code reads `selenium_animation_wait =
app.config["SCREENSHOT_SELENIUM_ANIMATION_WAIT"]` and unconditionally executes
`page.wait_for_timeout(selenium_animation_wait * 1000)` (Grep output at lines 340-348).
3. For a non-tiled screenshot (either `tiled_enabled` False, or `tiled_enabled` True with
`use_tiled` False) the flow continues into the branches shown in the PR hunk at
`superset/utils/webdriver.py:434-455` and `465-485`, where after waiting for `.loading`
spinners to disappear, the new code at lines 453-454 and 483-484 conditionally executes
`page.wait_for_timeout(selenium_animation_wait * 1000)` again when
`selenium_animation_wait > 0`.
4. With `SCREENSHOT_SELENIUM_ANIMATION_WAIT` configured to a non-zero value (e.g. `5`), a
single non-tiled Playwright screenshot now incurs two separate animation waits (first
before charts finish loading, then again after spinners clear), doubling the intended
delay for every PNG report and increasing end-to-end report generation time and timeout
risk compared to the single delay prior to this PR.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 453:454
**Comment:**
*Performance: This adds a second animation delay in the non-tiled flow, but the method already performs an unconditional `SCREENSHOT_SELENIUM_ANIMATION_WAIT` earlier before branching. With non-zero config values, screenshots now wait twice (effectively 2x the configured delay), which can significantly slow report generation and cause avoidable timeout pressure. Move the existing pre-branch wait to the post-spinner location (instead of adding a second wait), so the delay runs once at the correct time.
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 adds a second To implement this, remove the early superset/utils/webdriver.py |
1 participant
SUMMARY
ECharts charts run a ~1s draw animation that only starts once data has loaded and the spinner clears. In
WebDriverPlaywright.get_screenshot, the non-tiled (single chart / standard) path waitsSCREENSHOT_SELENIUM_ANIMATION_WAITbefore charts finish loading, then waits for.loadingspinners to disappear, then screenshots immediately. For a slow-loading chart the animation wait is spent while the chart is still loading, so the moment the spinner clears the screenshot fires mid-animation, producing a partially-rendered chart.This is the same root cause as #40694, which fixed only the tiled dashboard path (per-tile wait after the spinner clears). The non-tiled PNG path was never fixed — mix charts (bar + line, time-shift, multiple metrics) load slowest and are the most exposed, matching the customer report (3 of 53 reports affected → a timing race).
Fix: add the animation wait after the
.loadingspinner check in both non-tiled branches, gated onSCREENSHOT_SELENIUM_ANIMATION_WAIT > 0. Deployments already setting that value (e.g.=5) get it automatically; the default0is a no-op.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — bug manifests as a partially-drawn chart captured mid-animation; fix lets the draw animation complete before capture.
TESTING INSTRUCTIONS
SCREENSHOT_SELENIUM_ANIMATION_WAIT=5.Unit test:
pytest tests/unit_tests/utils/webdriver_test.py -k animation_wait_runs_after_spinners_clearADDITIONAL INFORMATION