fix(reports): positive readiness check for non-tiled screenshots - #42253
Conversation
dosubot
Bot
added
the
alert-reports
Code Review Agent Run #9f269eActionable Suggestions - 0Additional Suggestions - 1
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
The reported CodeQL issue regarding incomplete URL substring sanitization is not relevant to the changes in this pull request. The PR modifies screenshot utility functions in |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| CHART_HOLDERS_READY_JS = ( | ||
| f"() => {{ {UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0; }}" | ||
| ) |
There was a problem hiding this comment.
Suggestion: The readiness predicate still has a vacuous-pass path: it returns ready when zero chart holders are found. During initial React/bootstrap timing, the DOM can temporarily contain no holders, so the wait can complete immediately and still allow a blank/partial screenshot. Require at least one holder (or another explicit dashboard-ready signal) before returning ready. [logic error]
Severity Level: Critical 🚨
- ❌ Reports may show blank dashboards despite successful screenshot.
- ⚠️ Screenshot timeouts won't fire when no charts mounted.Steps of Reproduction ✅
1. Trigger any flow that uses `WebDriverPlaywright.get_screenshot()` at
`superset/utils/webdriver.py:338` with a dashboard URL (e.g. report generation or manual
invocation in tests).
2. In the non-tiled branch of `get_screenshot()` at `superset/utils/webdriver.py:493-547`,
`_wait_for_charts_ready()` at `superset/utils/webdriver.py:283-337` is called before
taking the standard screenshot.
3. `_wait_for_charts_ready()` calls `page.wait_for_function(CHART_HOLDERS_READY_JS,
timeout=load_wait * 1000)` at `superset/utils/webdriver.py:318-321`.
`CHART_HOLDERS_READY_JS` is defined at `superset/utils/screenshot_utils.py:119-121` to
execute `UNREADY_CHART_HOLDERS_JS_BODY` and then `return unready.length === 0`.
4. During the gap between page load and React dashboard bootstrap, the DOM can
legitimately contain no `[data-test="dashboard-component-chart-holder"]` elements (as
described in comments at `superset/utils/screenshot_utils.py:51-57`). In that state,
`document.querySelectorAll(...)` in `UNREADY_CHART_HOLDERS_JS_BODY` at
`superset/utils/screenshot_utils.py:81-83` returns an empty NodeList, the loop never
pushes into `unready`, `unready.length` remains `0`, and `CHART_HOLDERS_READY_JS`
evaluates to `true`. `page.wait_for_function` therefore returns immediately and
`get_screenshot()` proceeds to capture a blank or partially-loaded dashboard without ever
timing out or logging a warning.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 119:121
**Comment:**
*Logic Error: The readiness predicate still has a vacuous-pass path: it returns ready when zero chart holders are found. During initial React/bootstrap timing, the DOM can temporarily contain no holders, so the wait can complete immediately and still allow a blank/partial screenshot. Require at least one holder (or another explicit dashboard-ready signal) before returning ready.
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 #42253 +/- ##
==========================================
- Coverage 65.24% 65.23% -0.02%
==========================================
Files 2795 2795
Lines 157643 157694 +51
Branches 36061 36067 +6
==========================================
+ Hits 102853 102865 +12
- Misses 52814 52853 +39
Partials 1976 1976
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 #57a42fActionable 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 |
| hard_limit, soft_limit = timelimit | ||
| limit = soft_limit or hard_limit |
There was a problem hiding this comment.
Suggestion: Celery provides request.timelimit in soft-limit, hard-limit order, but this unpacking treats the first value as the hard limit and the second as the soft limit. When both limits are configured, the code therefore derives the budget from the hard limit and may continue past the soft limit, allowing Celery to interrupt the screenshot before cleanup completes. [logic error]
Severity Level: Major ⚠️
- ❌ Report screenshot tasks can exceed their soft deadline.
- ⚠️ Celery may interrupt browser cleanup or cache error handling.
- ⚠️ Dashboard report generation becomes less reliable near task limits.Steps of Reproduction ✅
1. Configure a report Celery task with distinct soft and hard limits through the Celery
task configuration referenced by `superset/config.py:1629`.
2. Trigger dashboard screenshot generation through `WebDriverPlaywright.get_screenshot()`
at `superset/utils/webdriver.py:409`, which calls
`resolve_screenshot_task_budget_seconds()` from `superset/utils/webdriver.py:338`.
3. Celery exposes `current_task.request.timelimit` as `(soft_limit, hard_limit)`, but
`superset/utils/screenshot_utils.py:61` assigns those values to `hard_limit` and
`soft_limit` in the opposite order.
4. At `superset/utils/screenshot_utils.py:62-69`, the calculation selects the actual hard
limit and reserves 20 percent, so the readiness wait can continue beyond the actual soft
deadline and Celery can interrupt the task during later cleanup or error handling.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 61:62
**Comment:**
*Logic Error: Celery provides `request.timelimit` in soft-limit, hard-limit order, but this unpacking treats the first value as the hard limit and the second as the soft limit. When both limits are configured, the code therefore derives the budget from the hard limit and may continue past the soft limit, allowing Celery to interrupt the screenshot before cleanup completes.
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| page.wait_for_function( | ||
| _TILE_READY_CHECK_JS, | ||
| CHART_HOLDERS_READY_JS, | ||
| timeout=load_wait * 1000, |
There was a problem hiding this comment.
Suggestion: The tiled path continues to use the full configured load_wait without deriving a remaining budget from the active Celery task. A task with a shorter soft or hard limit can therefore spend the entire static timeout waiting for each tile and be terminated before screenshot cleanup and error handling complete, unlike the non-tiled path. [possible bug]
Severity Level: Critical 🚨
- ❌ Tiled dashboard reports can time out before completion.
- ⚠️ Browser resources may not be cleaned up before termination.
- ⚠️ Cache error transitions may not execute reliably.Steps of Reproduction ✅
1. Trigger a dashboard screenshot through `WebDriverPlaywright.get_screenshot()` at
`superset/utils/webdriver.py:409` for a dashboard that exceeds the tiling threshold and
therefore calls `take_tiled_screenshot()` at `superset/utils/screenshot_utils.py:275`.
2. Use a Celery report task with a soft or hard time limit shorter than the configured
screenshot wait; the active task metadata is available through
`current_task.request.timelimit` and is otherwise handled by
`resolve_screenshot_task_budget_seconds()` at `superset/utils/screenshot_utils.py:42`.
3. For each tile, `take_tiled_screenshot()` at
`superset/utils/screenshot_utils.py:354-365` invokes `page.wait_for_function()` with the
full `load_wait` value and does not call the task-budget resolver.
4. If multiple tiles each wait for slow chart readiness, the cumulative waits can reach
the Celery deadline before `take_tiled_screenshot()` combines tiles or the report caller
performs cleanup, allowing Celery to terminate the task.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/screenshot_utils.py
**Line:** 364:366
**Comment:**
*Possible Bug: The tiled path continues to use the full configured `load_wait` without deriving a remaining budget from the active Celery task. A task with a shorter soft or hard limit can therefore spend the entire static timeout waiting for each tile and be terminated before screenshot cleanup and error handling complete, unlike the non-tiled path.
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| WebDriverPlaywright._wait_for_charts_ready( | ||
| page, | ||
| url, | ||
| self._screenshot_load_wait, | ||
| log_context=log_context, | ||
| screenshot_started_at=screenshot_started_at, |
There was a problem hiding this comment.
Suggestion: The task-budget calculation only limits the chart-readiness wait. The subsequent animation delay and screenshot operation can consume the remaining task budget, so the task may still hit its soft or hard deadline during capture despite this method reporting a safe effective wait. [possible bug]
Severity Level: Major ⚠️
- ❌ Screenshot tasks can be interrupted during final capture.
- ⚠️ Animation waits consume time reserved for cleanup.
- ⚠️ Report generation may fail after charts become ready.Steps of Reproduction ✅
1. Trigger standard screenshot generation through `WebDriverPlaywright.get_screenshot()`
at `superset/utils/webdriver.py:409`; the method records `screenshot_started_at` at line
416 and passes it to `_wait_for_charts_ready()` at lines 615-620.
2. `_wait_for_charts_ready()` at `superset/utils/webdriver.py:338-383` limits only the
chart-readiness polling timeout to the remaining task budget.
3. After that method returns, the same standard branch waits for `selenium_animation_wait`
at `superset/utils/webdriver.py:622-627` and then captures the image through
`_get_screenshot()` at `superset/utils/webdriver.py:283-287`.
4. If readiness completes close to the computed budget, the animation delay and screenshot
operation can consume the remaining time, causing Celery to interrupt capture or cleanup
even though the readiness wait itself respected the budget.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 615:620
**Comment:**
*Possible Bug: The task-budget calculation only limits the chart-readiness wait. The subsequent animation delay and screenshot operation can consume the remaining task budget, so the task may still hit its soft or hard deadline during capture despite this method reporting a safe effective wait.
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
Code Review Agent Run #1ed195Actionable Suggestions - 0Additional Suggestions - 3
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 |
The non-tiled/standard screenshot path in WebDriverPlaywright.get_screenshot() had the same vacuous-pass defect that #42119 fixed for tiled screenshots: it waited for document.querySelectorAll('.loading').length === 0, which is satisfied immediately if no chart has mounted a spinner yet (e.g. in the gap between page-navigation-complete and React/query bootstrap), silently capturing a blank screenshot with no timeout or error. Replace it with the same positive terminal-state predicate #42119 introduced for the tiled path (CHART_HOLDERS_READY_JS / FIND_UNREADY_CHART_HOLDERS_JS in screenshot_utils.py, now exported and reused verbatim instead of reimplemented) via a new WebDriverPlaywright._wait_for_charts_ready() helper, deduplicating the two copies of this wait that existed in get_screenshot(). On timeout it logs per-chart diagnostics (chart id + state) at WARNING and re-raises instead of capturing. Co-Authored-By: Claude <noreply@anthropic.com>
…y CodeQL CodeQL's "Incomplete URL substring sanitization" check flagged `"http://example.com" in warning_call[0]` as if it were unsafe URL substring matching. It's actually exact tuple-element membership against mock_logger.warning.call_args (not string substring matching), but the `in` syntax is ambiguous enough to read as the flagged anti-pattern to both CodeQL and human readers. Replace it, and one sibling occurrence with the same shape, with indexed equality checks against call_args.args instead. Co-Authored-By: Claude <noreply@anthropic.com>
54da85d to
62408be
Compare
Code Review Agent Run #2c0ebdActionable 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 |
|
Post-fold verification: #42383 is merged/folded into this PR. Its validated commit Scope precision for review: this fixes the #42253 staging regression and hardens non-tiled readiness (positive terminal states, chart-specific outer holder ID/selector excluding Markdown/DynamicComponent, Celery-derived remaining budget with cleanup margin, early/final diagnostics, and fail-before-capture cleanup). It does not establish or close the pre-existing PointFive SC-113788 root cause, which predates #42253. Corrected 6.0 staging validation: readiness 5.01s, full lifecycle 10.52s, cache Updated, and the chart-bearing PDF was visually confirmed. The earlier |
Co-authored-by: Matt Fitzgerald <matt.fitzgerald@preset.io>
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Reviewed at ddd9ad0b0 (backend/test files only — superset/config.py, superset/utils/screenshot_utils.py, superset/utils/webdriver.py, tests/unit_tests/utils/test_screenshot_utils.py, tests/unit_tests/utils/webdriver_test.py; confirmed via the compare API that no frontend files are part of this diff).
What checks out:
- Dedup is behavior-preserving. Diffed both old spinner-wait copies on
master(tiled_enabled=True+ small-dashboard branch, andtiled_enabled=Falsebranch — identicaltimeout=self._screenshot_load_wait*1000+ raise) against the newWebDriverPlaywright._wait_for_charts_ready(). Both call sites (webdriver.py:592andwebdriver.py:628) now pass identical arguments to the shared method, and when no Celery task budget is availableeffective_load_waitreduces to the oldload_waitexactly — no path silently diverges. - Positive readiness + fail-loud confirmed.
_wait_for_charts_ready(webdriver.py:291-421) replaces the vacuous.loading-absence check with the shared terminal-state predicate, and re-raises onPlaywrightTimeoutafter logging diagnostics — no path still captures a blank on timeout.test_chart_holder_with_nothing_mounted_does_not_satisfy_waitasserts the predicate string itself ("dashboard-component-chart-holder" in js), so it would correctly fail if reverted to the old check, not just assert-no-exception. - Viewport-scoping is sound. Confirmed the non-tiled branch never calls
page.set_viewport_size()(only the tiled branch does), so the viewport-intersection scoping in the shared predicate is the correct fix for the same reason as the tiled path. Explicitly tested (test_readiness_check_scoped_to_viewport_visible_holders). - Rename is clean — no leftover
_TILE_READY_CHECK_JS/_FIND_UNREADY_CHART_HOLDERS_JS/_UNREADY_CHART_HOLDERS_JS_BODYanywhere in the diff. One correction to my own review brief: the predicate body is not byte-identical to #42119's original, and that's a good thing. It independently verifies thatsuperset-frontend/babel.config.js'sproductionenv strips the baredata-testattribute viababel-plugin-jsx-remove-data-test-id— meaning[data-test="dashboard-component-chart-holder"](what #42119 shipped) matches nothing in a production build, so the tiled path's readiness check has likely been silently vacuous in production since #42119 merged. This PR switches to the pre-existingdashboard-chart-id-${chartId}class (already onChartHolder.tsxon master, unaffected by the plugin) for both paths, and addstest_readiness_constants_are_production_safeto guard it. Nice catch, worth calling out since it's a bigger deal than "rename." config.pychange is a doc-comment update only, consistent with the new check.
Open item, not blocking on its own but worth a response: UNREADY_CHART_HOLDERS_JS_BODY (screenshot_utils.py:121-151) still returns unready.length === 0 (i.e. ready) when document.querySelectorAll(CHART_HOLDER_SELECTOR) finds zero holders — this is unchanged from #42119's original logic on the tiled path, so it's not a regression introduced here. _wait_for_charts_ready now logs a warning for this case for element_name == "standalone" (webdriver.py:333), but doesn't raise or retry, so a capture that lands in that timing gap still silently proceeds. Given the whole point of this PR is closing exactly this class of silent-blank-capture bug, is a warning-only response intentional here, or worth raising ScreenshotTaskBudgetExceededError's sibling for this case too?
On the 4 unresolved codeant-ai threads, since I can't approve past unresolved bot threads regardless:
screenshot_utils.py:193(zero-holders vacuous pass) — real, see above; I'd call it a narrow pre-existing edge case rather than "Critical," but worth a reply either way.screenshot_utils.py:62(Celerytimelimitorder) — checked against installedcelery(amqp.py:459:'timelimit': (time_limit, soft_time_limit),task.py:146:limit_hard, limit_soft = self.timelimit) —hard_limit, soft_limit = timelimithere is correct. This one looks like a false positive.screenshot_utils.py:345andwebdriver.py:634(tiled path / animation+capture not budget-aware) — both are real scope observations, but match the PR description's explicit "NOT changed: the tiled path... untouched here" — reasonable to treat as follow-up rather than blocking this fix.
No blockers from me. Given the unresolved bot threads and the zero-holders question above, I'll leave this as a comment rather than approving — happy to approve once those are addressed or you confirm they're intentional/out of scope.
|
Thanks for the detailed review @aminghadersohi! The zero-holder warning-only behavior is intentional for this PR. A dashboard can legitimately contain no chart holders, for example, Markdown/dynamic-component-only dashboards; so treating zero holders as an unconditional readiness failure would make valid exports wait until the task budget expires and then fail. The warning makes the readiness-gate gap observable without introducing that regression. Fully closing the timing race requires a stronger signal that distinguishes “charts expected but not mounted yet” from “dashboard legitimately has zero charts,” likely using dashboard/layout metadata rather than DOM holder count alone. I’ll track that separately rather than broadening #42253. On the other CodeAnt threads:
|
|
Bito Automatic Review Skipped – PR Already Merged |
… hitting Celery kill Cumulative per-tile readiness waits in take_tiled_screenshot() have no bound tied to the running Celery task's time limit: each tile's wait_for_function runs at the full configured load_wait, so a slow dashboard with N tiles can wait up to N * load_wait and get SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the report failing cleanly and notifying owners. Reworked from this PR's original revision to sit on top of the merged #42253/#42427 readiness work and reuse its runtime budget helper rather than shipping a second one: - Derive one wall-clock budget for the whole tiled operation from resolve_screenshot_task_budget_seconds() (the #42427 helper the non-tiled path already uses). When it returns None (no Celery task context, e.g. synchronous thumbnail generation), fall back to a fixed total ceiling (TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440s) instead of "keep the configured timeout" -- unlike the non-tiled path's single wait, per-tile waits accumulate, so the operation still needs one bounded total. - Cap each tile's readiness-wait timeout at the remaining budget, recomputed after the mandatory scroll-settle sleep (which itself consumes wall-clock time) so a tile can't overrun by up to one settle interval. Cap or skip the cosmetic per-tile animation wait the same way. - Raise TiledScreenshotBudgetExceededError the moment the budget is exhausted -- before any further tile is captured -- and exempt it from the function's return-None fallback so callers fail the report loudly instead of receiving a partial/unchecked screenshot. No budget floor: a budget already exhausted by setup raises before the first tile, matching the non-tiled path's raise-before-capture semantics. - Make the new error a subclass of ScreenshotTaskBudgetExceededError (moved to screenshot_utils.py, re-exported from webdriver.py) so callers can catch the whole budget-error family with one type. - Enrich the per-tile timeout WARNING with budget context (waited vs requested load_wait, elapsed vs total budget, tiles captured) and add a per-tile DEBUG timing breakdown so slow dashboards can be profiled from logs alone. Co-Authored-By: Claude <noreply@anthropic.com>
… hitting Celery kill Cumulative per-tile readiness waits in take_tiled_screenshot() have no bound tied to the running Celery task's time limit: each tile's wait_for_function runs at the full configured load_wait, so a slow dashboard with N tiles can wait up to N * load_wait and get SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the report failing cleanly and notifying owners. Reworked from this PR's original revision to sit on top of the merged #42253/#42427 readiness work and reuse its runtime budget helper rather than shipping a second one: - Derive one wall-clock budget for the whole tiled operation from resolve_screenshot_task_budget_seconds() (the #42427 helper the non-tiled path already uses). When it returns None (no Celery task context, e.g. synchronous thumbnail generation), fall back to a fixed total ceiling (TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440s) instead of "keep the configured timeout" -- unlike the non-tiled path's single wait, per-tile waits accumulate, so the operation still needs one bounded total. - Cap each tile's readiness-wait timeout at the remaining budget, recomputed after the mandatory scroll-settle sleep (which itself consumes wall-clock time) so a tile can't overrun by up to one settle interval. Cap or skip the cosmetic per-tile animation wait the same way. - Raise TiledScreenshotBudgetExceededError the moment the budget is exhausted -- before any further tile is captured -- and exempt it from the function's return-None fallback so callers fail the report loudly instead of receiving a partial/unchecked screenshot. No budget floor: a budget already exhausted by setup raises before the first tile, matching the non-tiled path's raise-before-capture semantics. - Make the new error a subclass of ScreenshotTaskBudgetExceededError (moved to screenshot_utils.py, re-exported from webdriver.py) so callers can catch the whole budget-error family with one type. - Enrich the per-tile timeout WARNING with budget context (waited vs requested load_wait, elapsed vs total budget, tiles captured) and add a per-tile DEBUG timing breakdown so slow dashboards can be profiled from logs alone. Co-Authored-By: Claude <noreply@anthropic.com>
… hitting Celery kill Cumulative per-tile readiness waits in take_tiled_screenshot() have no bound tied to the running Celery task's time limit: each tile's wait_for_function runs at the full configured load_wait, so a slow dashboard with N tiles can wait up to N * load_wait and get SIGKILLed mid-capture (SoftTimeLimitExceeded) instead of the report failing cleanly and notifying owners. Reworked from this PR's original revision to sit on top of the merged #42253/#42427 readiness work and reuse its runtime budget helper rather than shipping a second one: - Derive one wall-clock budget for the whole tiled operation from resolve_screenshot_task_budget_seconds() (the #42427 helper the non-tiled path already uses). When it returns None (no Celery task context, e.g. synchronous thumbnail generation), fall back to a fixed total ceiling (TILED_SCREENSHOT_TOTAL_WAIT_BUDGET_SECONDS = 1440s) instead of "keep the configured timeout" -- unlike the non-tiled path's single wait, per-tile waits accumulate, so the operation still needs one bounded total. - Cap each tile's readiness-wait timeout at the remaining budget, recomputed after the mandatory scroll-settle sleep (which itself consumes wall-clock time) so a tile can't overrun by up to one settle interval. Cap or skip the cosmetic per-tile animation wait the same way. - Raise TiledScreenshotBudgetExceededError the moment the budget is exhausted -- before any further tile is captured -- and exempt it from the function's return-None fallback so callers fail the report loudly instead of receiving a partial/unchecked screenshot. No budget floor: a budget already exhausted by setup raises before the first tile, matching the non-tiled path's raise-before-capture semantics. - Make the new error a subclass of ScreenshotTaskBudgetExceededError (moved to screenshot_utils.py, re-exported from webdriver.py) so callers can catch the whole budget-error family with one type. - Enrich the per-tile timeout WARNING with budget context (waited vs requested load_wait, elapsed vs total budget, tiles captured) and add a per-tile DEBUG timing breakdown so slow dashboards can be profiled from logs alone. Co-Authored-By: Claude <noreply@anthropic.com>
…eenshot capture logs The screenshot capture code is reached by two call paths that identify their runs differently: scheduled reports carry an execution_id (already threaded end-to-end via log_context since #42253), while thumbnails and direct PDF/screenshot downloads are identified by their cache_key -- which was never passed down. BaseScreenshot.compute_and_cache had the cache_key in hand and get_screenshot already accepted a log_context parameter, but the two were never connected, so every capture-layer log line produced by a thumbnail or direct-download run is anonymous: there is no way to join "trying to generate screenshot" / webdriver navigation / readiness / capture-result log lines to the cached digest they were computing. This threads the existing optional log_context through the remaining capture-layer log lines, and populates it on the thumbnail path: - screenshots.py: compute_and_cache passes log_context=f"cache_key={cache_key}" into get_screenshot and resize_image; the thumbnail lifecycle log lines (generate/fail/resize/ cache-updated) now include the cache_key; driver() accepts log_context for its Playwright-unavailable fallback notice. - webdriver.py: the non-tiled Playwright log lines (navigation, headstart, element/chart-container waits, screenshot result), the entire WebDriverSelenium.get_screenshot path, and find_unexpected_errors (both engines) now append the context suffix. - screenshot_utils.py: the non-budget tiled log lines (dimensions, tile count, scroll, capture, skip, combine) and combine_screenshot_tiles gain the same suffix. Log-line/plumbing only -- no behavior change. Split out of #42118 per its scope reduction to tiled-path budgeting; the readiness log lines added by Co-Authored-By: Claude <noreply@anthropic.com>
4 participants
Decisions made that were not in the instructions
WebDriverPlaywright.get_screenshot()'s non-tiled branches never callpage.set_viewport_size()before capturing (that call only exists in the tiled branch, to resize totile_height) — the browser viewport stays at the configured window size (e.g.DEFAULT_DASHBOARD_WINDOW_SIZE = 1600, 1200) for the whole capture.element.screenshot()/page.screenshot(full_page=True)can capture below-the-fold content without ever scrolling the page or resizing the viewport, soDashboardVirtualization'sIntersectionObserver-based placeholders below the fold never mount anything real before the screenshot is taken. Requiring all chart holders (not just viewport-visible ones) to reach a terminal state would therefore deadlock on those by-design placeholders. This mirrors the tiled path's exact reasoning.screenshot_utils.py's_TILE_READY_CHECK_JS/_FIND_UNREADY_CHART_HOLDERS_JS/_UNREADY_CHART_HOLDERS_JS_BODY(added in fix(reports): positive per-tile chart readiness check for tiled screenshots #42119) already implement exactly the terminal-state check this fix needs. They're renamed to drop the leading underscore (CHART_HOLDERS_READY_JS,FIND_UNREADY_CHART_HOLDERS_JS,UNREADY_CHART_HOLDERS_JS_BODY) and imported intowebdriver.pyrather than reimplemented, so the two capture paths can't drift apart.get_screenshot()had two near-identical copies of the old spinner wait (one fortiled_enabled=True+ small dashboard, one fortiled_enabled=False). Both are replaced by a single newWebDriverPlaywright._wait_for_charts_ready()static method to avoid maintaining the fix twice.SUMMARY
#42119 fixed a "vacuous pass" bug in the tiled screenshot path: the per-tile spinner check only looked for the absence of currently-mounted
.loadingelements, so a chart holder that hadn't started rendering yet (nothing mounted, no spinner) passed the check immediately and got captured blank.The identical defect was still present, unfixed, in the non-tiled/standard capture path.
WebDriverPlaywright.get_screenshot()had its own global spinner wait using the same old predicate:If no chart has mounted a
.loadingelement yet at the exact moment this check first runs (a timing gap between page-navigation-complete and React/query bootstrap), the predicate is satisfied immediately and silently — no timeout, no warning, no error. The result is a structurally valid PNG that's just visually blank.This PR replaces that predicate with the same positive terminal-state check #42119 introduced for the tiled path: every viewport-intersecting
[data-test="dashboard-component-chart-holder"]must show either a rendered chart ([data-test="slice-container"], no nested.loading) or an error/empty state ([role="alert"],.ant-empty,.missing-chart-container). On timeout, it now logs a WARNING (not ERROR — a customer chart-loading issue, not a system fault, matching #42119's convention) with per-chart diagnostics (chart id + why it's unready:nothing_mounted/waiting_on_database/spinner_mounted) and re-raises, instead of the previous generic "timed out waiting for charts to load" message with no diagnostics.log_context(already threaded throughget_screenshotfor the tiled call site) is threaded into the new non-tiled wait too, for log correlation.This is the suspected root cause of a customer-reported blank-screenshot issue on dashboard PDF downloads: a structurally valid but visually blank capture, with no corresponding error anywhere, consistent with a silent capture-time vacuous pass rather than a loud failure.
NOT changed
take_tiled_screenshotinsuperset/utils/screenshot_utils.py) — already fixed by fix(reports): positive per-tile chart readiness check for tiled screenshots #42119, untouched here except for the constant rename (no behavior change).TESTING INSTRUCTIONS
tests/unit_tests/utils/webdriver_test.py:test_uses_wait_for_function_to_detect_spinners/test_spinner_timeout_logs_warning_and_raisesupdated to assert the new predicate and diagnostics instead of the old absence-of-.loadingstring.TestWebDriverPlaywrightChartReadinessclass:test_chart_holder_with_nothing_mounted_does_not_satisfy_wait— regression test for the vacuous-pass fix; asserts no screenshot is captured.test_all_chart_holders_ready_passes— happy path, screenshot is returned.test_readiness_check_scoped_to_viewport_visible_holders— asserts the JS predicate is viewport-scoped (getBoundingClientRect/window.innerHeight) and thatset_viewport_sizeis never called on this path, proving the viewport-scoping finding above.test_log_context_threaded_into_readiness_wait— assertslog_contextappears in the timeout warning.test_spinner_timeout_logs_warning_and_raises(updated) — asserts WARNING (not ERROR) level with per-chart diagnostics on timeout.tests/unit_tests/utils/test_screenshot_utils.pyupdated for the constant rename only (no behavior change to the tiled path).ruff check/ruff formatandmypyon both modified source files; no new mypy errors versus master (confirmed by diffing mypy output before/after the change — same 3 pre-existing errors, only line numbers shifted).pytest tests/unit_tests/utils/webdriver_test.py tests/unit_tests/utils/test_screenshot_utils.py— 64 passed.ADDITIONAL INFORMATION