fix(screenshots): catch empty-bytes tiled result and set ERROR on falsy image#41097
fix(screenshots): catch empty-bytes tiled result and set ERROR on falsy image#41097eschutho wants to merge 5 commits into
Conversation
Code Review Agent Run #e84413Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| def test_cache_error_status_when_screenshot_returns_empty_bytes( | ||
| self, mocker: MockerFixture, screenshot_obj, mock_user | ||
| ): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, | ||
| } | ||
|
|
||
| def _make_pw_mocks(self, mock_sync_playwright): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def test_animation_wait_after_spinner_wait_tiled_disabled( | ||
| self, mock_app, mock_sync_playwright | ||
| ): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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|
|
||
| call_order: list[str] = [] | ||
|
|
||
| def record_wait_for_function(*args, **kwargs): |
There was a problem hiding this comment.
Suggestion: Add type annotations for *args, **kwargs, and the return type in this newly introduced local function. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def record_wait_for_timeout(ms): | ||
| if ms == 2 * 1000: | ||
| call_order.append("animation_wait") |
There was a problem hiding this 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
Why it matters? 🤔
This newly added local function has no docstring. The custom rule requires newly added Python functions and classes to include docstrings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41097 +/- ##
==========================================
- Coverage 64.34% 63.76% -0.59%
==========================================
Files 2653 2653
Lines 144952 144966 +14
Branches 33433 33436 +3
==========================================
- Hits 93272 92434 -838
- Misses 49996 50846 +850
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #a30f76Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
eschutho
force-pushed
the
reports-screenshot-api-spinner-wait
branch
from
769a4ad to
f032d83
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #cf58ee
Actionable Suggestions - 1
-
superset/utils/screenshots.py - 1
- Duplicate error() call · Line 315-316
Review Details
-
Files reviewed - 5 · Commit Range:
14c1bce..f032d83- pyproject.toml
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/test_screenshot_cache_fix.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| else: | ||
| cache_payload.error() |
There was a problem hiding this comment.
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.
Code suggestion
Check the AI-generated fix before applying
--- 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())
Code Review Run #cf58ee
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, | ||
| } | ||
|
|
||
| def _make_pw_mocks(self, mock_sync_playwright): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def test_animation_wait_after_spinner_wait_tiled_disabled( | ||
| self, mock_app, mock_sync_playwright | ||
| ): |
There was a problem hiding this 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
Why it matters? 🤔
This newly added test method has untyped parameters and no return annotation.
The custom rule explicitly flags new Python methods that omit type hints.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def test_tiled_path_passes_animation_wait_per_tile_no_global_wait( | ||
| self, mock_app, mock_take_tiled, mock_sync_playwright | ||
| ): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def test_tiled_fallback_triggered_on_empty_bytes( | ||
| self, mock_app, mock_take_tiled, mock_sync_playwright | ||
| ): |
There was a problem hiding this comment.
Suggestion: Add complete type hints for this method's parameters and declare its return type explicitly. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The method is newly added and has untyped parameters with no return annotation.
This is a genuine match for the custom typing rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
There was a problem hiding this comment.
Code Review Agent Run #f070b0
Actionable Suggestions - 1
-
superset/utils/screenshots.py - 1
- Missing test for timestamp preservation · Line 315-318
Review Details
-
Files reviewed - 4 · Commit Range:
f032d83..4619241- pyproject.toml
- superset/utils/screenshots.py
- tests/unit_tests/utils/test_screenshot_cache_fix.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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() |
There was a problem hiding this comment.
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.
Code Review Run #f070b0
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
…ht non-tiled path ECharts animations play after chart data loads (after the loading spinner clears). The Playwright screenshot path was applying the global SCREENSHOT_SELENIUM_ANIMATION_WAIT *before* waiting for spinners to clear, meaning the animation buffer was consumed before data had even loaded. Move the animation wait to after `wait_for_function` (the global spinner check) in both non-tiled code paths — when SCREENSHOT_TILED_ENABLED is False, and when it is True but the dashboard is below the tiling threshold. The tiled path already handles this correctly per-tile inside `take_tiled_screenshot`, which was fixed by PRs #39895 and #40694. This brings the thumbnail API / Celery screenshot path to parity with the report-email path on small-to-medium dashboards. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…right path Add debug-level logging at each decision point in WebDriverPlaywright so that enabling DEBUG logging exposes the full screenshot pipeline: - Which path was taken (tiled / standard-below-threshold / tiled-disabled) including chart count and dashboard height when the tiling decision is made - "Waiting for all spinners to clear" with SCREENSHOT_LOAD_WAIT value at the start of each wait, so a timeout message has clear context - "All spinners cleared" confirmation after wait_for_function succeeds - "Taking screenshot of url %s as user %s" immediately before capture, rather than at the misleading early position it previously occupied - "Screenshot result: N bytes" (or "Tiled screenshot result: N bytes") after capture so a blank or suspiciously small result is visible in the log Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sy image Two bugs that could cause blank PDFs to re-trigger indefinitely: 1. webdriver.py: tiled fallback used `if img is None:` which let b"" (returned by combine_screenshot_tiles on an empty tile list) pass through silently. Changed to `if not img:` to catch both cases. 2. screenshots.py: compute_and_cache had no else branch after `if image: cache_payload.update(image)`. When get_screenshot returned None or b"" without raising, status stayed at COMPUTING instead of ERROR, causing is_computing_stale() to re-trigger the task indefinitely after THUMBNAIL_ERROR_CACHE_TTL. Added `else: cache_payload.error()` so failed screenshots always transition to ERROR and use the controlled TTL back-off. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lback test Add PT001 to ruff ignore list so local and CI ruff versions don't conflict over fixture parentheses style. Also fix test_tiled_fallback_triggered_on_empty_bytes to patch page.screenshot directly instead of the static method, which was returning a MagicMock in CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename and update test_tiled_screenshot_failure_returns_none_without_fallback to reflect new behavior: falsy tiled result falls back to standard screenshot instead of returning None - Guard the else: cache_payload.error() call in compute_and_cache to only fire when status is not already ERROR, preserving the original failure timestamp - Remove stale PT004 ignore (rule removed in ruff 0.9.7) - Apply ruff 0.9.7 formatting to match CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eschutho
force-pushed
the
reports-screenshot-api-spinner-wait
branch
from
4619241 to
3d626c8
Compare
2 participants
SUMMARY
Fixes two bugs that caused screenshot tasks to re-trigger indefinitely when tiled screenshots produced blank results.
Bug 1 — empty-bytes tiled fallback (
webdriver.py)combine_screenshot_tiles([])returnsb""when the tile list is empty. The old guardif img is None:let empty bytes pass through silently, skipping the standard screenshot fallback entirely. Changed toif not img:so bothNoneandb""trigger the fallback.Bug 2 — COMPUTING status never cleared on falsy image (
screenshots.py)When
get_screenshotreturnedNoneorb""without raising,compute_and_cachefinished with the task still inCOMPUTINGstate (set at the start, never updated). OnceTHUMBNAIL_ERROR_CACHE_TTLelapsed,is_computing_stale()fired and re-triggered the task — indefinitely. Addedelse: cache_payload.error()so any falsy image result transitions toERRORand respects the TTL back-off.Together these two bugs formed a silent retry loop: the tiled path returned empty bytes → the fallback was skipped → the task completed with no image and COMPUTING status → the stale-computing check re-triggered the task → repeat.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only bug fixes.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Part of the blank-PDF investigation series on this branch (follows the animation-wait ordering fix and debug logging additions).
🤖 Generated with Claude Code