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.34% -0.54%
==========================================
Files 2582 2583 +1
Lines 136413 136610 +197
Branches 31453 31504 +51
==========================================
- Hits 87136 86537 -599
- Misses 47764 48557 +793
- 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 |
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