codeant-ai-for-open-source[bot] commented on code in PR #41428:
URL: https://github.com/apache/superset/pull/41428#discussion_r3476070048
##########
superset/utils/webdriver.py:
##########
@@ -444,6 +444,14 @@ def get_screenshot( # pylint: disable=too-many-locals,
too-many-statements # n
self._screenshot_load_wait,
)
raise
+ # Wait for chart animations (e.g. ECharts) to finish
+ # after the spinners clear. The earlier animation wait
+ # runs before charts finish loading, but the draw
+ # animation only starts once data arrives and the
+ # spinner clears, so without this wait a slow-loading
+ # chart is captured mid-render.
+ if selenium_animation_wait > 0:
+ page.wait_for_timeout(selenium_animation_wait *
1000)
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Non-tiled Playwright PNG reports wait double animation time.
- ⚠️ Report generation slower, increased timeout pressure in jobs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0d7a3f95129b4488bf182a8a2f5f1fff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0d7a3f95129b4488bf182a8a2f5f1fff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41428&comment_hash=ef4e1e122911bbd2208ab4e8c0a3120e7656ad81909c01d3dc57d19b33ea0938&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41428&comment_hash=ef4e1e122911bbd2208ab4e8c0a3120e7656ad81909c01d3dc57d19b33ea0938&reaction=dislike'>👎</a>
--
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]
