fix(reports): narrow spinner checks to viewport and tighten exception handling#39892
Open
fix(reports): narrow spinner checks to viewport and tighten exception handling#39892
Conversation
…g elements
The Playwright screenshot path waited for `.loading` elements to detach
by calling `page.locator(".loading").all()` once and iterating the
snapshot. Spinners that appeared after the snapshot was taken (common on
long dashboards where charts load lazily) were never waited for, causing
PDF reports to capture charts that were still fetching data.
Replace the snapshot loop with `page.wait_for_function` which
continuously polls `document.querySelectorAll('.loading').length === 0`
until the page is genuinely clear of all spinners, then use
`self._screenshot_load_wait` (already stored on the base class) for the
timeout to stay consistent with the Selenium path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… 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>
Contributor
Code Review Agent Run #c27d0dActionable 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 |
dosubot
Bot
added
the
alert-reports
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Follow-up to #39579. The first fix addressed the snapshot-based spinner detection in
webdriver.py. This PR addresses additional issues identified during review:Viewport-aware global spinner check — The global
wait_for_functioninwebdriver.pypreviously checkeddocument.querySelectorAll('.loading').length === 0, which can deadlock on tall dashboards whenDashboardVirtualizationrenders off-screen placeholder.loadingdivs. Changed to the samegetBoundingClientRectviewport-visibility check used in the per-tile path.Narrow exception handling in
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 — Thetake_tiled_screenshotload_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