fix(reports): narrow spinner checks to viewport and tighten exception handling#39895
fix(reports): narrow spinner checks to viewport and tighten exception handling#39895
Conversation
… handling - Replace global '.loading' count check in webdriver.py with a getBoundingClientRect viewport-visibility check to avoid deadlock when DashboardVirtualization renders off-screen placeholder spinners - Narrow except clause in screenshot_utils.py from bare Exception to PlaywrightTimeout so non-timeout errors (e.g. browser crash) propagate - Fix load_wait default from 30 s to 60 s to match SCREENSHOT_LOAD_WAIT config default - Add tests covering per-tile spinner wait, timeout warning, non-timeout propagation, and load_wait default value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
alert-reports
![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 #19541f
Actionable Suggestions - 1
-
superset/utils/webdriver.py - 1
- Semantic duplication in loading visibility logic · Line 304-313
Additional Suggestions - 1
-
superset/utils/screenshot_utils.py - 1
-
Incorrect Documentation · Line 100-100The docstring for `load_wait` incorrectly states the default as 30, but the parameter defaults to 60 and the new test confirms this matches the SCREENSHOT_LOAD_WAIT config default. This could mislead users reading the documentation.
Code suggestion
@@ -99,2 +99,2 @@ - tile_height: Height of each tile in pixels - load_wait: Seconds to wait for charts to load per tile (default 30) + tile_height: Height of each tile in pixels + load_wait: Seconds to wait for charts to load per tile (default 60)
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/utils/screenshot_utils.py - 1
- Docstring default mismatch · Line 100-100
Review Details
-
Files reviewed - 4 · Commit Range:
4e6ab1c..4e6ab1c- superset/utils/screenshot_utils.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/test_screenshot_utils.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
| """() => { | ||
| const els = document.querySelectorAll('.loading'); | ||
| for (const el of els) { | ||
| const r = el.getBoundingClientRect(); | ||
| if (r.top < window.innerHeight && r.bottom > 0) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }""", |
There was a problem hiding this comment.
The updated loading element visibility check logic duplicates the same JavaScript code already present in take_tiled_screenshot within screenshot_utils.py. This creates maintenance risk if the logic needs updates, as changes would need to be synced across both files. Consider refactoring to a shared utility.
Code Review Run #19541f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39895 +/- ##
==========================================
+ Coverage 63.87% 63.89% +0.01%
==========================================
Files 2582 2583 +1
Lines 136413 136616 +203
Branches 31453 31503 +50
==========================================
+ Hits 87136 87284 +148
- Misses 47764 47816 +52
- Partials 1513 1516 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Include SCREENSHOT_LOAD_WAIT / load_wait in the warning message so it is immediately visible from logs whether the timeout limit was reached. Also fix stale docstring default value (30 -> 60). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left a few notes below - these look like CI/readiness issues around the new timeout handling and tests. All line numbers verified against HEAD 8d7ffac.
Functional - worth fixing before merge
-
superset/utils/screenshot_utils.py:224The new test at
tests/unit_tests/utils/test_screenshot_utils.py:368expects non-timeoutwait_for_functionerrors to propagate, buttake_tiled_screenshotstill wraps the whole function inexcept Exceptionand returnsNone. In environments where Playwright is not importable,PlaywrightTimeout = Exceptionat line 34 also makes the inner timeout handler catch every exception, so aRuntimeError("browser crashed")is treated as a spinner timeout instead of matching the test's contract.WDYT - could we either make non-timeout errors actually escape this helper, or update the test/PR description if the intended behavior is still "fall back to the standard screenshot"?
-
tests/unit_tests/utils/test_screenshot_utils.py:358This assertion still expects the old warning call with only the tile index and tile count, but the production logger call now includes the
(load_wait=%ss)suffix and passesload_waitas a fourth argument atsuperset/utils/screenshot_utils.py:170-174. The focused unit test fails on this mismatch.Small suggestion: could we update the expected
assert_any_callto include the new message and30timeout argument? -
tests/unit_tests/utils/webdriver_test.py:755Similar stale assertion here:
superset/utils/webdriver.py:318-322now logs theSCREENSHOT_LOAD_WAITsuffix and passesself._screenshot_load_wait, but the test still expects the previous two-argument warning call.Could we update this expected call too, so the test tracks the new logging signature?
Praise
-
superset/utils/webdriver.py:383Passing
self._screenshot_load_waitintotake_tiled_screenshotkeeps the tiled and non-tiled Playwright paths aligned with the same configured timeout, which looks like the right direction for this follow-up.
Code Review Agent Run #bf07f7Actionable 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 |
- Update assert_any_call in per-tile spinner timeout test to include the new (load_wait=%ss) suffix and value - Update assert_any_call in webdriver spinner timeout test to include the new (SCREENSHOT_LOAD_WAIT=%ss) suffix and value - Fix test_per_tile_non_timeout_exceptions: RuntimeError is caught by the outer take_tiled_screenshot handler and returns None, not re-raised; rename and update assertion to match actual behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ndent behavior The test outcome depends on whether playwright is importable at test time. When playwright is absent, PlaywrightTimeout aliases to Exception, so the inner except clause catches RuntimeError and the screenshot proceeds. When playwright is present the outer handler catches it and returns None. Remove the test rather than encoding environment-specific behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #be817bActionable Suggestions - 0Filtered 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 |
2 participants
SUMMARY
Follow-up to #39579. After that fix landed, a review identified additional issues:
Viewport-aware global spinner check (
webdriver.py) — The globalwait_for_functioncheckeddocument.querySelectorAll('.loading').length === 0, which deadlocks on tall dashboards whenDashboardVirtualizationrenders off-screen placeholder.loadingdivs. Changed to agetBoundingClientRectviewport-visibility check — the same approach already used in the per-tile path.Narrow exception handling (
screenshot_utils.py) — The per-tile spinner wait caught bareException, silently swallowing non-timeout errors (e.g. browser crash). Narrowed toPlaywrightTimeoutwith a runtime-safe conditional import (same try/except import pattern aswebdriver.py).Fix
load_waitdefault —take_tiled_screenshot'sload_waitparameter defaulted to 30 s, inconsistent with theSCREENSHOT_LOAD_WAITconfig default of 60 s.Tests — Added tests for the per-tile spinner wait: viewport JS check, timeout warning + continue, non-timeout propagation, and the
load_waitdefault value. Updated existing webdriver test to match the new viewport-aware JS.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend logic change only.
TESTING INSTRUCTIONS
pytest tests/unit_tests/utils/test_screenshot_utils.py tests/unit_tests/utils/webdriver_test.pyADDITIONAL INFORMATION