codeant-ai-for-open-source[bot] commented on code in PR #40694:
URL: https://github.com/apache/superset/pull/40694#discussion_r3345290485
##########
superset/utils/screenshot_utils.py:
##########
@@ -174,6 +176,12 @@ def take_tiled_screenshot(
load_wait,
)
+ # Wait for chart animations (e.g. ECharts) to finish after spinner
clears.
+ # The global animation wait before tiling only covers the first
tile;
+ # subsequent tiles need their own wait after data loads.
+ if animation_wait > 0:
+ page.wait_for_timeout(animation_wait * 1000)
Review Comment:
**Suggestion:** The per-tile animation delay runs even after a tile already
hit `load_wait` timeout waiting for spinners, so each failing tile incurs an
additional delay on top of an already-expired load wait. This compounds
failure-path latency and can make report jobs exceed runtime limits; only apply
the animation wait when the spinner check actually succeeded. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard thumbnail tasks take longer when tiles time out.
- ⚠️ Tiled Playwright PDFs risk exceeding Celery soft time limits.
- ⚠️ Report latency increases on dashboards with stuck loading spinners.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable Playwright screenshots via feature flag
`PLAYWRIGHT_REPORTS_AND_THUMBNAILS` and
configure tiled screenshots so large dashboards use tiling (see
`superset/utils/screenshots.py:14-18` where `WebDriverPlaywright` is
selected and
`superset/utils/webdriver.py:238-319` where `use_tiled` determines tiled
capture).
2. Trigger a dashboard thumbnail/report generation for a large dashboard
(e.g. Celery task
`cache_dashboard_screenshot` in `superset/tasks/thumbnails.py:22-32` which
constructs
`DashboardScreenshot` and calls `get_screenshot()`, which in turn calls
`WebDriverPlaywright.get_screenshot()` in
`superset/utils/webdriver.py:226-235`).
3. Configure or encounter a dashboard state where, for at least one tile, a
`.loading`
spinner never disappears or takes longer than `SCREENSHOT_LOAD_WAIT` (wired
into
`self._screenshot_load_wait`) so that the per-tile wait for visible spinners
in
`take_tiled_screenshot()` times out: `page.wait_for_function(...,
timeout=load_wait *
1000)` at `superset/utils/screenshot_utils.py:88-100` raises
`PlaywrightTimeout` and is
caught at `170-177` with a warning logged.
4. Observe that after this timeout, regardless of the failure, the code
unconditionally
applies the per-tile animation delay when `animation_wait > 0` at
`superset/utils/screenshot_utils.py:179-183` (`if animation_wait > 0:
page.wait_for_timeout(animation_wait * 1000)`), so each timed-out tile
incurs an extra
`animation_wait` seconds on top of the already-expired `load_wait`,
extending overall job
duration on the error path without improving rendering.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8bad2cd86784fad98cbaf53d34bde41&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f8bad2cd86784fad98cbaf53d34bde41&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/screenshot_utils.py
**Line:** 182:183
**Comment:**
*Logic Error: The per-tile animation delay runs even after a tile
already hit `load_wait` timeout waiting for spinners, so each failing tile
incurs an additional delay on top of an already-expired load wait. This
compounds failure-path latency and can make report jobs exceed runtime limits;
only apply the animation wait when the spinner check actually succeeded.
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%2F40694&comment_hash=ce4af9e176737ee088e74f595397ec2b16530c61fddfeb286b4e1617e13e1ea0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40694&comment_hash=ce4af9e176737ee088e74f595397ec2b16530c61fddfeb286b4e1617e13e1ea0&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]
