codeant-ai-for-open-source[bot] commented on code in PR #42253:
URL: https://github.com/apache/superset/pull/42253#discussion_r3618757123
##########
superset/utils/screenshot_utils.py:
##########
@@ -104,14 +116,14 @@
# Predicate for page.wait_for_function: true once every viewport-visible chart
# holder has reached a terminal state.
-_TILE_READY_CHECK_JS = (
- f"() => {{ {_UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0;
}}"
+CHART_HOLDERS_READY_JS = (
+ f"() => {{ {UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0; }}"
)
Review Comment:
**Suggestion:** The readiness predicate still has a vacuous-pass path: it
returns ready when zero chart holders are found. During initial React/bootstrap
timing, the DOM can temporarily contain no holders, so the wait can complete
immediately and still allow a blank/partial screenshot. Require at least one
holder (or another explicit dashboard-ready signal) before returning ready.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Reports may show blank dashboards despite successful screenshot.
- ⚠️ Screenshot timeouts won't fire when no charts mounted.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger any flow that uses `WebDriverPlaywright.get_screenshot()` at
`superset/utils/webdriver.py:338` with a dashboard URL (e.g. report
generation or manual
invocation in tests).
2. In the non-tiled branch of `get_screenshot()` at
`superset/utils/webdriver.py:493-547`,
`_wait_for_charts_ready()` at `superset/utils/webdriver.py:283-337` is
called before
taking the standard screenshot.
3. `_wait_for_charts_ready()` calls
`page.wait_for_function(CHART_HOLDERS_READY_JS,
timeout=load_wait * 1000)` at `superset/utils/webdriver.py:318-321`.
`CHART_HOLDERS_READY_JS` is defined at
`superset/utils/screenshot_utils.py:119-121` to
execute `UNREADY_CHART_HOLDERS_JS_BODY` and then `return unready.length ===
0`.
4. During the gap between page load and React dashboard bootstrap, the DOM
can
legitimately contain no `[data-test="dashboard-component-chart-holder"]`
elements (as
described in comments at `superset/utils/screenshot_utils.py:51-57`). In
that state,
`document.querySelectorAll(...)` in `UNREADY_CHART_HOLDERS_JS_BODY` at
`superset/utils/screenshot_utils.py:81-83` returns an empty NodeList, the
loop never
pushes into `unready`, `unready.length` remains `0`, and
`CHART_HOLDERS_READY_JS`
evaluates to `true`. `page.wait_for_function` therefore returns immediately
and
`get_screenshot()` proceeds to capture a blank or partially-loaded dashboard
without ever
timing out or logging a warning.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e572485b1b164a1f96a883cb0c57fc12&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=e572485b1b164a1f96a883cb0c57fc12&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:** 119:121
**Comment:**
*Logic Error: The readiness predicate still has a vacuous-pass path: it
returns ready when zero chart holders are found. During initial React/bootstrap
timing, the DOM can temporarily contain no holders, so the wait can complete
immediately and still allow a blank/partial screenshot. Require at least one
holder (or another explicit dashboard-ready signal) before returning 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%2F42253&comment_hash=719369849af0e48a7a2ffa0653577220c209bafde1ff851f84d55ada3467a44a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42253&comment_hash=719369849af0e48a7a2ffa0653577220c209bafde1ff851f84d55ada3467a44a&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]
