You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Gordonei (via GitHub)" <gi...@apache.org> on 2025/09/06 10:44:08 UTC
[PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Gordonei opened a new pull request, #35041:
URL: https://github.com/apache/superset/pull/35041
(cherry picked from commit 2233c02720f30e01198e2e8a1367902d29c59729)
### SUMMARY
This small PR just back ports the change described in https://github.com/apache/superset/pull/33107 to `4.1` branch
### TESTING INSTRUCTIONS
When `4.1.x` worker with playwright enable screenshots an empty dashboard or a slice without a graph:
```
```
[2025-09-05 06:27:29,501: ERROR/ForkPoolWorker-6] Timed out requesting url https://<redacted>/superset/slice/3129/?standalone=true
Traceback (most recent call last):
File "/app/superset/utils/webdriver.py", line 182, in get_screenshot
element.wait_for()
File "/usr/local/lib/python3.10/site-packages/playwright/sync_api/_generated.py", line 17975, in wait_for
self._sync(self._impl_obj.wait_for(timeout=timeout, state=state))
File "/usr/local/lib/python3.10/site-packages/playwright/_impl/_sync_base.py", line 115, in _sync
return task.result()
File "/usr/local/lib/python3.10/site-packages/playwright/_impl/_locator.py", line 692, in wait_for
await self._frame.wait_for_selector(
File "/usr/local/lib/python3.10/site-packages/playwright/_impl/_frame.py", line 369, in wait_for_selector
await self._channel.send(
File "/usr/local/lib/python3.10/site-packages/playwright/_impl/_connection.py", line 69, in send
return await self._connection.wrap_api_call(
File "/usr/local/lib/python3.10/site-packages/playwright/_impl/_connection.py", line 558, in wrap_api_call
raise rewrite_error(error, f"{parsed_st['apiName']}: {error}") from None
playwright._impl._errors.TimeoutError: Locator.wait_for: Timeout 120000ms exceeded.
Call log:
- waiting for locator(".chart-container") to be visible
```
### ADDITIONAL INFORMATION
- [X] Has associated issue: https://github.com/apache/superset/issues/33132
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35041:
URL: https://github.com/apache/superset/pull/35041#issuecomment-3267235396
Thanks for pointing this out. We'll close the PR since that's not how we pick/backport cherries. We've labeled the original PR so it gets picked up in the next patch release for 4.1.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35041:
URL: https://github.com/apache/superset/pull/35041#discussion_r2326894331
##########
superset/utils/webdriver.py:
##########
@@ -188,7 +188,6 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements
# chart containers didn't render
logger.debug("Wait for chart containers to draw at url: %s", url)
slice_container_locator = page.locator(".chart-container")
- slice_container_locator.first.wait_for()
for slice_container_elem in slice_container_locator.all():
slice_container_elem.wait_for()
Review Comment:
### Missing Chart Container Verification <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code now waits for all chart containers without first verifying that at least one exists. This can cause the loop to execute zero times, bypassing necessary waiting for chart containers to render.
###### Why this matters
If a dashboard is expected to have charts, they might not be fully rendered before taking the screenshot, resulting in blank or incomplete visualizations in the captured image.
###### Suggested change ∙ *Feature Preview*
Add a verification step before the loop to ensure charts are properly handled when present:
```python
slice_container_locator = page.locator(".chart-container")
if slice_container_locator.count() > 0: # Only wait for charts if they exist
for slice_container_elem in slice_container_locator.all():
slice_container_elem.wait_for()
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfbc957f-2f8c-4360-839d-e05131fef651/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfbc957f-2f8c-4360-839d-e05131fef651?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfbc957f-2f8c-4360-839d-e05131fef651?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfbc957f-2f8c-4360-839d-e05131fef651?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfbc957f-2f8c-4360-839d-e05131fef651)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:39ab063a-4f7f-4714-8aa3-0ddb9976c305 -->
[](39ab063a-4f7f-4714-8aa3-0ddb9976c305)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35041:
URL: https://github.com/apache/superset/pull/35041#issuecomment-3261817172
**Bito Automatic Review Skipped - Branch Excluded**<br/><br/>Bito didn't auto-review because the source or target branch is excluded from automatic reviews.<br/>No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type `/review` in a comment and save.<br/>You can change the branch exclusion settings [here](https://alpha.bito.ai/home/ai-agents/code-review-agent), or contact your Bito workspace admin at [evan@preset.io](mailto:evan@preset.io).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #35041: fix(playwright): allow screenshotting empty dashboards (#33107)
URL: https://github.com/apache/superset/pull/35041
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "Gordonei (via GitHub)" <gi...@apache.org>.
Gordonei commented on PR #35041:
URL: https://github.com/apache/superset/pull/35041#issuecomment-3267251357
> Thanks for pointing this out. We'll close the PR since that's not how we pick/backport cherries. We've labeled the original PR so it gets picked up in the next patch release for 4.1.
Sure, happy that it's will be in the next release
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(playwright): allow screenshotting empty dashboards (#33107) [superset]
Posted by "Gordonei (via GitHub)" <gi...@apache.org>.
Gordonei commented on PR #35041:
URL: https://github.com/apache/superset/pull/35041#issuecomment-3264649415
@mistercrunch @hxtmdev - to bring to your attention, since you were involved in the previous PR
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org