codeant-ai-for-open-source[bot] commented on code in PR #43348:
URL: https://github.com/apache/superset/pull/43348#discussion_r3817781900
##########
superset/utils/webdriver.py:
##########
@@ -382,7 +384,19 @@ def _wait_for_charts_ready( # noqa: C901
if element_name == "chart-container":
readiness_predicate = CHART_CONTAINER_READY_JS
elif report_execution_context:
- readiness_predicate = REPORT_CHART_HOLDERS_READY_JS
+ # This non-tiled path captures the whole element in one shot
+ # (`_get_screenshot` uses `full_page=True` /
`element.screenshot()`),
+ # so below-the-fold holders end up in the image. Force every
+ # virtualized row to render up front -- mirroring the client-side
+ # "Download as Image/PDF" path -- and then require *all* mounted
+ # holders (not just the viewport-visible ones) to reach a terminal
+ # state. If an off-screen holder never renders, the wait times out
+ # and the report fails loudly instead of silently delivering a
+ # blank/partial screenshot as a Success. The tiled path keeps the
+ # viewport-scoped predicate because it scrolls each region into
view
+ # before capturing it.
+ page.evaluate(FORCE_ALL_CHART_HOLDERS_IN_VIEW_JS)
+ readiness_predicate = REPORT_ALL_CHART_HOLDERS_READY_JS
Review Comment:
**Suggestion:** The report readiness predicate only requires the currently
mounted holders to be ready. Because dispatching the force-render event updates
React state asynchronously, the existing mounted set can satisfy this predicate
before additional virtualized rows mount, allowing the full-page screenshot to
proceed while later charts are still absent. Require the mounted holder count
to reach `report_execution_context.expected_chart_count` and then verify all
holders are ready. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Non-tiled scheduled PDFs can omit later virtualized charts.
- ⚠️ Reports may falsely transition to successful delivery.
- ⚠️ The race affects forced off-screen chart mounting.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<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:** 398:399
**Comment:**
*Api Mismatch: The report readiness predicate only requires the
currently mounted holders to be ready. Because dispatching the force-render
event updates React state asynchronously, the existing mounted set can satisfy
this predicate before additional virtualized rows mount, allowing the full-page
screenshot to proceed while later charts are still absent. Require the mounted
holder count to reach `report_execution_context.expected_chart_count` and then
verify all holders are ready.
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%2F43348&comment_hash=512f9b840c36debad4a316da9f2b0d2e37109707376eaca1ef9802acb29c63ed&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43348&comment_hash=512f9b840c36debad4a316da9f2b0d2e37109707376eaca1ef9802acb29c63ed&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]
