You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/06/16 00:32:06 UTC
[PR] fix(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
eschutho opened a new pull request, #41097:
URL: https://github.com/apache/superset/pull/41097
### SUMMARY
Two bugs that could cause blank PDFs to re-trigger screenshot tasks indefinitely:
**Bug 1 — `webdriver.py` tiled fallback (`if img is None:` → `if not img:`):**
`combine_screenshot_tiles([])` returns `b""` (falsy, not `None`) when the tile list is empty. The old check `if img is None:` let empty bytes pass through silently without triggering the standard screenshot fallback. Changed to `if not img:` to catch both `None` and `b""`.
**Bug 2 — `screenshots.py` `compute_and_cache` (add `else: cache_payload.error()`):**
When `get_screenshot` returned `None` or `b""` without raising an exception, the task finished with status still at `COMPUTING` (set at the start of the task). After `THUMBNAIL_ERROR_CACHE_TTL` elapsed, `is_computing_stale()` returned `True` and re-triggered the task indefinitely. The fix adds `else: cache_payload.error()` so any falsy image result always transitions to `ERROR` and uses the controlled TTL back-off instead of the stale-COMPUTING loop.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only bug fixes.
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightAnimationWaitOrder::test_tiled_fallback_triggered_on_empty_bytes`
- `pytest tests/unit_tests/utils/test_screenshot_cache_fix.py::TestCacheOnlyOnSuccess::test_cache_error_status_when_screenshot_returns_empty_bytes`
- Run full suite: `pytest tests/unit_tests/utils/webdriver_test.py tests/unit_tests/utils/test_screenshot_cache_fix.py tests/unit_tests/utils/test_screenshot_utils.py`
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
Part of the blank-PDF investigation series (follows the animation-wait ordering fix and debug logging additions on this branch).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4794030384
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3417398288
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com",
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
Review Comment:
**Suggestion:** Add explicit type annotations for the helper method parameters and return value so the new method is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code and the helper method has no type hints for its parameter or return value. The custom rule requires new Python functions/methods to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=43cdc4731198456a953012303a5ed8fd&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=43cdc4731198456a953012303a5ed8fd&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 920:920
**Comment:**
*Custom Rule: Add explicit type annotations for the helper method parameters and return value so the new method is fully typed.
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%2F41097&comment_hash=a41e2cae4b75ff84657a96be5ef4a04eb01388cfdc5bc72a40beaadb0f806dfa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=a41e2cae4b75ff84657a96be5ef4a04eb01388cfdc5bc72a40beaadb0f806dfa&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com",
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add type hints for all parameters and the return type of this new test method to comply with full typing requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method, and its parameters and return value are not type-annotated. The rule says new Python code should be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f4468a46350457fb68a10a7a5aada07&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=9f4468a46350457fb68a10a7a5aada07&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 940:942
**Comment:**
*Custom Rule: Add type hints for all parameters and the return type of this new test method to comply with full typing requirements.
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%2F41097&comment_hash=0b054e6148ec631c804ae3d39eea2092e8a3dcecf47ae028924762d7cea3aa85&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=0b054e6148ec631c804ae3d39eea2092e8a3dcecf47ae028924762d7cea3aa85&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/test_screenshot_cache_fix.py:
##########
@@ -149,6 +149,25 @@ def test_cache_saved_only_when_image_generated(
assert cached_value["status"] == "Updated"
assert cached_value["image"] is not None
+ def test_cache_error_status_when_screenshot_returns_empty_bytes(
+ self, mocker: MockerFixture, screenshot_obj, mock_user
+ ):
Review Comment:
**Suggestion:** Add full type annotations to this newly added test method by typing all parameters and the return value (`-> None`) so new code is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test method in the changed hunk, and it lacks type hints for
`screenshot_obj`, `mock_user`, and the return value. The custom rule requires new
Python code to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87297c65b7674c5f9d1be1df283f832d&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=87297c65b7674c5f9d1be1df283f832d&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:** tests/unit_tests/utils/test_screenshot_cache_fix.py
**Line:** 152:154
**Comment:**
*Custom Rule: Add full type annotations to this newly added test method by typing all parameters and the return value (`-> None`) so new code is fully typed.
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%2F41097&comment_hash=66cdb6ef04a206e095b17e1916108b6640baddce4a530f8800b78a7fa23bc820&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=66cdb6ef04a206e095b17e1916108b6640baddce4a530f8800b78a7fa23bc820&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com",
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
Review Comment:
**Suggestion:** Add type annotations for `*args`, `**kwargs`, and the return type in this newly introduced local function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added local function omits type annotations for `*args`, `**kwargs`, and the return type. The custom rule requires new Python functions to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db74f7549d644c38989de2b7d40dff19&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=db74f7549d644c38989de2b7d40dff19&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 952:952
**Comment:**
*Custom Rule: Add type annotations for `*args`, `**kwargs`, and the return type in this newly introduced local function.
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%2F41097&comment_hash=493ef1061110c5653f62f52341b022f6adea09d54a92d93f492f769d4879d6f5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=493ef1061110c5653f62f52341b022f6adea09d54a92d93f492f769d4879d6f5&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -898,3 +898,229 @@ def evaluate_side_effect(script):
"dashboard",
"http://example.com",
)
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
Review Comment:
**Suggestion:** Add a docstring to this newly added local function so its role in call-order tracking is documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added local function has no docstring. The custom rule requires newly added Python functions and classes to include docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c37ebf47a574c4eb2c7781d5befea4b&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=9c37ebf47a574c4eb2c7781d5befea4b&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 955:957
**Comment:**
*Custom Rule: Add a docstring to this newly added local function so its role in call-order tracking is documented.
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%2F41097&comment_hash=e9f1c05f639fa377c021a4755042661eb243ee0cb2e4fd9290aa8bc20c16bfee&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=e9f1c05f639fa377c021a4755042661eb243ee0cb2e4fd9290aa8bc20c16bfee&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: 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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #41097:
URL: https://github.com/apache/superset/pull/41097
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4773286624
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3439988227
##########
superset/utils/screenshots.py:
##########
@@ -312,6 +312,8 @@ def compute_and_cache( # pylint: disable=too-many-arguments
if image:
with event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
cache_payload.update(image)
+ else:
+ cache_payload.error()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate error() call</b></div>
<div id="fix">
The new `else` block calls `error()` which overwrites the timestamp even when `error()` was already called earlier at line 302 or 308. This masks the actual error timing. Add a guard to check if status is already ERROR before calling error() again.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/utils/screenshots.py (lines 312-319)---
312: if image:
313: with event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
314: cache_payload.update(image)
315: elif cache_payload.status != StatusValues.ERROR:
316: cache_payload.error()
317:
318: logger.info("Caching thumbnail: %s", cache_key)
319: self.cache.set(cache_key, cache_payload.to_dict())
```
</div>
</details>
</div>
<small><i>Code Review Run #cf58ee</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3455596346
##########
superset/utils/screenshots.py:
##########
@@ -312,6 +312,10 @@ def compute_and_cache( # pylint: disable=too-many-arguments
if image:
with event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
cache_payload.update(image)
+ elif cache_payload.status != StatusValues.ERROR:
+ # Only call error() if not already set — avoids overwriting the timestamp
+ # recorded when the actual failure occurred (line 302 or 308 above).
+ cache_payload.error()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for timestamp preservation</b></div>
<div id="fix">
The fix logic is sound, but add a test that captures the specific scenario: get_screenshot returns an image, then resize fails. Verify error() is only called once (at line 308) and the timestamp reflects the resize failure, not a later overwrite.
</div>
</div>
<small><i>Code Review Run #f070b0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4813441131
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4734779442
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 769a4ad833d48df583491ff82441d4ff6e08fa73 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a32fea27f1c230008d007d5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41097--superset-docs-preview.netlify.app](https://deploy-preview-41097--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctNCoAgEEDhu8w6YTL6oRt0jElHEkqHUYiI7p67t_jeC6L5kgorQAe7UnJHa2XJWospTplTOXI1JNEUiSmxmptibdyznPnZfBsmGmxgsnPonR0QcfGIsx_h-wE) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41097--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4775193443
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4747962166
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3454608907
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -961,13 +961,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
Review Comment:
**Suggestion:** Add explicit parameter and return type annotations to this helper method so the new test utility is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The helper method is newly added and lacks both parameter type hints and a return type annotation.
That matches the custom rule requiring new Python methods to be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94c73cc022bc463384c4ed838b780477&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=94c73cc022bc463384c4ed838b780477&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 991:991
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to this helper method so the new test utility is fully typed.
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%2F41097&comment_hash=1dcebd5d23e89ead649a4453ba059747207eea07286a5aa43cefeffd7de5909f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=1dcebd5d23e89ead649a4453ba059747207eea07286a5aa43cefeffd7de5909f&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -961,13 +961,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ spinner_idx = call_order.index("spinner_wait")
+ anim_idx = call_order.index("animation_wait")
+ assert spinner_idx < anim_idx, (
+ "spinner wait must precede animation wait in non-tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Small dashboard: 3 charts, 1000px height — below both thresholds
+ mock_page.evaluate.side_effect = [3, 1000]
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
+ """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Large dashboard: 25 charts, 6000px height
+ mock_page.evaluate.side_effect = [25, 6000]
+ mock_take_tiled.return_value = b"tiled_screenshot"
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ result = WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "standalone", mock_user
+ )
+
+ assert result == b"tiled_screenshot"
+ mock_take_tiled.assert_called_once_with(
+ mock_page,
+ "standalone",
+ 600,
+ load_wait=30,
+ animation_wait=2,
+ )
+ # The only wait_for_timeout call should be the 0ms headstart; no global
+ # animation wait should be issued (handled per-tile by take_tiled_screenshot)
+ animation_waits = [
+ call[0][0]
+ for call in mock_page.wait_for_timeout.call_args_list
+ if call[0][0] == 2 * 1000
+ ]
+ assert animation_waits == [], (
+ "No global 2s animation wait_for_timeout should fire on the tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_fallback_triggered_on_empty_bytes(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add complete type hints for this method's parameters and declare its return type explicitly. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The method is newly added and has untyped parameters with no return annotation.
This is a genuine match for the custom typing rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd574255d15640dc9a9bf9e5445e33b0&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=bd574255d15640dc9a9bf9e5445e33b0&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1141:1143
**Comment:**
*Custom Rule: Add complete type hints for this method's parameters and declare its return type explicitly.
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%2F41097&comment_hash=b6895afd77476dce6643100e24251775213aadd4107321235b575298ec97816c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=b6895afd77476dce6643100e24251775213aadd4107321235b575298ec97816c&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -961,13 +961,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add type hints for the mocked parameters and an explicit return type annotation on this new test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method has untyped parameters and no return annotation.
The custom rule explicitly flags new Python methods that omit type hints.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53a2eff8d79b4a09aa7d1b811aa3f315&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=53a2eff8d79b4a09aa7d1b811aa3f315&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1011:1013
**Comment:**
*Custom Rule: Add type hints for the mocked parameters and an explicit return type annotation on this new test method.
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%2F41097&comment_hash=512f7c22d7151360e2bd6ad9bdad1a72a275835b5b60a7c80318bd594047e2b7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=512f7c22d7151360e2bd6ad9bdad1a72a275835b5b60a7c80318bd594047e2b7&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -961,13 +961,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ spinner_idx = call_order.index("spinner_wait")
+ anim_idx = call_order.index("animation_wait")
+ assert spinner_idx < anim_idx, (
+ "spinner wait must precede animation wait in non-tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Small dashboard: 3 charts, 1000px height — below both thresholds
+ mock_page.evaluate.side_effect = [3, 1000]
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add type annotations for all parameters and an explicit return type to this new test method to satisfy full typing requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced test method and it lacks both parameter annotations and a return type.
That is directly covered by the typing rule for new Python code.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=961881f595c949afba7754e3b478cb22&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=961881f595c949afba7754e3b478cb22&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1093:1095
**Comment:**
*Custom Rule: Add type annotations for all parameters and an explicit return type to this new test method to satisfy full typing requirements.
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%2F41097&comment_hash=50e363838ab5dd0dc4fa7b7f1f1e815a68f7251dd25a38cb73c75400aaa61203&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=50e363838ab5dd0dc4fa7b7f1f1e815a68f7251dd25a38cb73c75400aaa61203&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: 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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3483212732
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
Review Comment:
**Suggestion:** Add explicit type hints for the parameters and return value of this helper so new test code remains fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code and it omits both parameter type hints and a return type annotation.
The custom rule requires new Python functions/methods to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f7073a4db9c041728668fd7bf8cbc6e7&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=f7073a4db9c041728668fd7bf8cbc6e7&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 994:994
**Comment:**
*Custom Rule: Add explicit type hints for the parameters and return value of this helper so new test code remains fully typed.
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%2F41097&comment_hash=6ed4d8c17d11b756811b9fb5eddd90047b107cfc89d138e2d36d6fe46c7e08e8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=6ed4d8c17d11b756811b9fb5eddd90047b107cfc89d138e2d36d6fe46c7e08e8&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add a return type annotation and parameter type hints to this newly added test method to comply with the typing rule for new Python code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method omits parameter annotations and a return annotation.
Since the surrounding code is being changed, the custom typing rule applies and this is a genuine violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2b7f072a2eec4ac88c38bd38e6a06dd5&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=2b7f072a2eec4ac88c38bd38e6a06dd5&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1014:1016
**Comment:**
*Custom Rule: Add a return type annotation and parameter type hints to this newly added test method to comply with the typing rule for new Python code.
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%2F41097&comment_hash=b8dec38244db9bfcd19cc6e9b2d2b41efe3ff5dfb430d1419d930b2897e09c82&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=b8dec38244db9bfcd19cc6e9b2d2b41efe3ff5dfb430d1419d930b2897e09c82&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ spinner_idx = call_order.index("spinner_wait")
+ anim_idx = call_order.index("animation_wait")
+ assert spinner_idx < anim_idx, (
+ "spinner wait must precede animation wait in non-tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Small dashboard: 3 charts, 1000px height — below both thresholds
+ mock_page.evaluate.side_effect = [3, 1000]
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
Review Comment:
**Suggestion:** Add complete type annotations to this new test method signature, including its return type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is new Python test code and it is not typed: the parameters are unannotated and there is no return type.
That matches the custom rule for new or modified Python code, so the suggestion is verified.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7cd9bb45eb6f4b29b1b5427cdbd77d1c&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=7cd9bb45eb6f4b29b1b5427cdbd77d1c&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1096:1098
**Comment:**
*Custom Rule: Add complete type annotations to this new test method signature, including its return type.
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%2F41097&comment_hash=653c1eac6e1fd19f4fab2a74086fb154e9a5c2b42fcc33821551732f3dc8c3e4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=653c1eac6e1fd19f4fab2a74086fb154e9a5c2b42fcc33821551732f3dc8c3e4&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,240 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_sync_playwright):
+ mock_playwright_instance = MagicMock()
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_sync_playwright.return_value.__enter__.return_value = (
+ mock_playwright_instance
+ )
+ mock_playwright_instance.chromium.launch.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ spinner_idx = call_order.index("spinner_wait")
+ anim_idx = call_order.index("animation_wait")
+ assert spinner_idx < anim_idx, (
+ "spinner wait must precede animation wait in non-tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+ self, mock_app, mock_sync_playwright
+ ):
+ """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Small dashboard: 3 charts, 1000px height — below both thresholds
+ mock_page.evaluate.side_effect = [3, 1000]
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
+ """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+
+ # Large dashboard: 25 charts, 6000px height
+ mock_page.evaluate.side_effect = [25, 6000]
+ mock_take_tiled.return_value = b"tiled_screenshot"
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ result = WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "standalone", mock_user
+ )
+
+ assert result == b"tiled_screenshot"
+ mock_take_tiled.assert_called_once_with(
+ mock_page,
+ "standalone",
+ 600,
+ load_wait=30,
+ animation_wait=2,
+ )
+ # The only wait_for_timeout call should be the 0ms headstart; no global
+ # animation wait should be issued (handled per-tile by take_tiled_screenshot)
+ animation_waits = [
+ call[0][0]
+ for call in mock_page.wait_for_timeout.call_args_list
+ if call[0][0] == 2 * 1000
+ ]
+ assert animation_waits == [], (
+ "No global 2s animation wait_for_timeout should fire on the tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_fallback_triggered_on_empty_bytes(
+ self, mock_app, mock_take_tiled, mock_sync_playwright
+ ):
+ """Tiled fallback fires when take_tiled_screenshot returns b"" (not None)."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
+ mock_page.evaluate.side_effect = [25, 6000]
+ # Empty bytes — falsy but not None; was silently passed through before the fix
+ mock_take_tiled.return_value = b""
+ # _get_screenshot("standalone") calls page.screenshot(full_page=True);
+ # configure that return value so we can assert the fallback was reached
+ mock_page.screenshot.return_value = b"fallback"
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ result = WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "standalone", mock_user
+ )
+
+ assert result == b"fallback"
+ # Tiled path was taken (take_tiled_screenshot was called)
+ mock_take_tiled.assert_called_once()
+ # Standard screenshot was called as fallback (full_page=True for "standalone")
+ mock_page.screenshot.assert_called_with(full_page=True)
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver.sync_playwright")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_skipped_when_zero(self, mock_app, mock_sync_playwright):
Review Comment:
**Suggestion:** Add type hints for method parameters and an explicit `None` return annotation on this new test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method lacks parameter type hints and a return annotation.
The custom rule explicitly requires new Python functions and classes to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2e103522d2c541a18890412893458fa2&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=2e103522d2c541a18890412893458fa2&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1180:1180
**Comment:**
*Custom Rule: Add type hints for method parameters and an explicit `None` return annotation on this new test method.
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%2F41097&comment_hash=6fe2a0cf8bdd40ba15a8bbded22d774a2ddb0583e5dc01b6454630f98187165f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=6fe2a0cf8bdd40ba15a8bbded22d774a2ddb0583e5dc01b6454630f98187165f&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: 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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4814984090
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41097:
URL: https://github.com/apache/superset/pull/41097#discussion_r3484698277
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,236 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_browser_manager):
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_browser_manager.get_browser.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_browser_manager
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
Review Comment:
**Suggestion:** Add parameter and return type hints for this newly added nested function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added nested function has no type annotations for its parameters or return value, so it violates the typing rule for new Python functions.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d4d822c9f05c4d549ecf97ded04fbca4&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=d4d822c9f05c4d549ecf97ded04fbca4&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1022:1022
**Comment:**
*Custom Rule: Add parameter and return type hints for this newly added nested function.
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%2F41097&comment_hash=340e2ff48dac6d8ba7570b1f27c5c5ef80bb38c39853249e84c39a4814a7fb7a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=340e2ff48dac6d8ba7570b1f27c5c5ef80bb38c39853249e84c39a4814a7fb7a&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,236 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_browser_manager):
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_browser_manager.get_browser.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_browser_manager
+ ):
Review Comment:
**Suggestion:** Add type annotations for all parameters and an explicit return type for this newly added test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method omits type hints for its parameters and return value, which matches the typing rule for new Python methods.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8efb54f7a615444f8c7c1649398f222a&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=8efb54f7a615444f8c7c1649398f222a&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1010:1012
**Comment:**
*Custom Rule: Add type annotations for all parameters and an explicit return type for this newly added test method.
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%2F41097&comment_hash=478bc5242211ff6207ec0cd43862edff0c1ded4583bc6426052872ffc5abf67a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=478bc5242211ff6207ec0cd43862edff0c1ded4583bc6426052872ffc5abf67a&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -964,13 +964,236 @@ def evaluate_side_effect(script):
driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
- "http://example.com", "dashboard", mock_user
+ "http://example.com", "standalone", mock_user
)
- assert result is None
- mock_element.screenshot.assert_not_called()
- mock_logger.error.assert_any_call(
- "Tiled screenshot failed at url %s; "
- "not falling back to avoid sending a blank PDF",
- "http://example.com",
+ assert result == b"fallback_screenshot"
+ mock_take_tiled.assert_called_once()
+ mock_logger.warning.assert_any_call(
+ ("Tiled screenshot failed, falling back to standard screenshot"),
+ )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+ """Animation wait must run after the spinner wait, not before."""
+
+ _base_config = {
+ "WEBDRIVER_OPTION_ARGS": [],
+ "WEBDRIVER_WINDOW": {"pixel_density": 1},
+ "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+ "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+ "SCREENSHOT_SELENIUM_HEADSTART": 0,
+ "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+ "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+ "SCREENSHOT_LOCATE_WAIT": 10,
+ "SCREENSHOT_LOAD_WAIT": 30,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+ "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+ }
+
+ def _make_pw_mocks(self, mock_browser_manager):
+ mock_browser = MagicMock()
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_element = MagicMock()
+
+ mock_browser_manager.get_browser.return_value = mock_browser
+ mock_browser.new_context.return_value = mock_context
+ mock_context.new_page.return_value = mock_page
+ mock_page.locator.return_value = mock_element
+ mock_element.screenshot.return_value = b"screenshot"
+ return mock_context, mock_page
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_disabled(
+ self, mock_app, mock_browser_manager
+ ):
+ """Non-tiled path: animation wait runs after spinner wait_for_function."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+ mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ spinner_idx = call_order.index("spinner_wait")
+ anim_idx = call_order.index("animation_wait")
+ assert spinner_idx < anim_idx, (
+ "spinner wait must precede animation wait in non-tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+ self, mock_app, mock_browser_manager
+ ):
+ """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+ # Small dashboard: 3 charts, 1000px height — below both thresholds
+ mock_page.evaluate.side_effect = [3, 1000]
+
+ call_order: list[str] = []
+
+ def record_wait_for_function(*args, **kwargs):
+ call_order.append("spinner_wait")
+
+ def record_wait_for_timeout(ms):
+ if ms == 2 * 1000:
+ call_order.append("animation_wait")
+
+ mock_page.wait_for_function.side_effect = record_wait_for_function
+ mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "test-element", mock_user
+ )
+
+ assert "spinner_wait" in call_order
+ assert "animation_wait" in call_order
+ assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+ self, mock_app, mock_take_tiled, mock_browser_manager
+ ):
+ """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+ # Large dashboard: 25 charts, 6000px height
+ mock_page.evaluate.side_effect = [25, 6000]
+ mock_take_tiled.return_value = b"tiled_screenshot"
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ result = WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "standalone", mock_user
+ )
+
+ assert result == b"tiled_screenshot"
+ mock_take_tiled.assert_called_once_with(
+ mock_page,
+ "standalone",
+ 600,
+ load_wait=30,
+ animation_wait=2,
+ )
+ # The only wait_for_timeout call should be the 0ms headstart; no global
+ # animation wait should be issued (handled per-tile by take_tiled_screenshot)
+ animation_waits = [
+ call[0][0]
+ for call in mock_page.wait_for_timeout.call_args_list
+ if call[0][0] == 2 * 1000
+ ]
+ assert animation_waits == [], (
+ "No global 2s animation wait_for_timeout should fire on the tiled path"
+ )
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.take_tiled_screenshot")
+ @patch("superset.utils.webdriver.app")
+ def test_tiled_fallback_triggered_on_empty_bytes(
+ self, mock_app, mock_take_tiled, mock_browser_manager
+ ):
+ """Tiled fallback fires when take_tiled_screenshot returns b"" (not None)."""
+ mock_user = MagicMock()
+ mock_user.username = "test_user"
+ mock_app.config = {
+ **self._base_config,
+ "SCREENSHOT_TILED_ENABLED": True,
+ "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+ "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+ "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+ }
+
+ mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+ mock_page.evaluate.side_effect = [25, 6000]
+ # Empty bytes — falsy but not None; was silently passed through before the fix
+ mock_take_tiled.return_value = b""
+ # _get_screenshot("standalone") calls page.screenshot(full_page=True);
+ # configure that return value so we can assert the fallback was reached
+ mock_page.screenshot.return_value = b"fallback"
+
+ with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+ result = WebDriverPlaywright("chrome").get_screenshot(
+ "http://example.com", "standalone", mock_user
+ )
+
+ assert result == b"fallback"
+ # Tiled path was taken (take_tiled_screenshot was called)
+ mock_take_tiled.assert_called_once()
+ # Standard screenshot was called as fallback (full_page=True for "standalone")
+ mock_page.screenshot.assert_called_with(full_page=True)
+
+ @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+ @patch("superset.utils.webdriver._browser_manager")
+ @patch("superset.utils.webdriver.app")
+ def test_animation_wait_skipped_when_zero(self, mock_app, mock_browser_manager):
Review Comment:
**Suggestion:** Add type annotations for all parameters and a `None` return type on this newly added test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method omits type hints for its parameters and return type, so it is a real violation of the new-code typing rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d42778d00f6d46569d9e57568341e47f&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=d42778d00f6d46569d9e57568341e47f&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:** tests/unit_tests/utils/webdriver_test.py
**Line:** 1176:1176
**Comment:**
*Custom Rule: Add type annotations for all parameters and a `None` return type on this newly added test method.
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%2F41097&comment_hash=9d3411958f1e32712483265cfd0885c377bd366005acc6626f62602eb3d8c90d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41097&comment_hash=9d3411958f1e32712483265cfd0885c377bd366005acc6626f62602eb3d8c90d&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: 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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4736344532
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4713797192
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41097?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.47%. Comparing base ([`c1b5d05`](https://app.codecov.io/gh/apache/superset/commit/c1b5d05f83b361004a1eb9c61d9d79d1ef89d23e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a099655`](https://app.codecov.io/gh/apache/superset/commit/a09965557d8c577603ae734ee458965a52a0f323?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41097?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/webdriver.py](https://app.codecov.io/gh/apache/superset/pull/41097?src=pr&el=tree&filepath=superset%2Futils%2Fwebdriver.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvd2ViZHJpdmVyLnB5) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/screenshots.py](https://app.codecov.io/gh/apache/superset/pull/41097?src=pr&el=tree&filepath=superset%2Futils%2Fscreenshots.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2NyZWVuc2hvdHMucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41097 +/- ##
==========================================
- Coverage 64.26% 63.47% -0.79%
==========================================
Files 2660 2660
Lines 144795 144808 +13
Branches 33384 33386 +2
==========================================
- Hits 93048 91914 -1134
- Misses 50096 51237 +1141
- Partials 1651 1657 +6
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.34% <0.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.91% <0.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.91% <0.00%> (-1.69%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.75% <0.00%> (-0.02%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41097?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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(screenshots): catch empty-bytes tiled result and set ERROR on falsy image [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41097:
URL: https://github.com/apache/superset/pull/41097#issuecomment-4713725908
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large 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