fix(reports): tile readiness check backport on 6.0-release-era base (report-fixes-42119) - #42278
Closed
eschutho wants to merge 1140 commits into
Closed
fix(reports): tile readiness check backport on 6.0-release-era base (report-fixes-42119)#42278eschutho wants to merge 1140 commits into
eschutho wants to merge 1140 commits into
Conversation
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit cf587ca)
(cherry picked from commit ad5e317)
(cherry picked from commit 4311a15)
(cherry picked from commit 09c7e1f) (cherry picked from commit bf1eb3d57e4d10a6c0a5ad15e8cab498c49b006f)
Adds CSV and XLSX download buttons plus an explicit reload control to the drill-to-detail and drill-by modals. Adds a shared DownloadDropdown component used by both modals. Backport of #37109 from master to 6.0-release. Adapted for 6.0-release: - All @apache-superset/core/* imports collapsed back to @superset-ui/core (the namespace refactor is master-only). - canCopyClipboard from usePermissions is master-only; gated the new copy-disabled tooltip on canDownload instead until canCopyClipboard is ported. - TabularDataRow type is master-only (utils/common is still .js); inlined as Record<string, unknown>[]. - The Explore pane Results/Samples tabs receive onDownloadCSV/ onDownloadXLSX/onReload as optional props but the row-limit selector changes from master are intentionally omitted (master moved that area toward a different pattern that is not present on 6.0-release). - Backend FRONTEND_CONF_KEYS gains ROW_LIMIT so the new drill download payload can read it from common.conf. (cherry picked from commits fa31ec8, 967fa7b, 9ba8fda, b39419c, b74cc05, 15be40b, d8c8849, 6e1f03b, ca0e9cf, 76cdc1f) Co-Authored-By: alex-poor <alex@karo.co.nz>
(cherry picked from commit c2b9272)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit e4fe08a)
) (cherry picked from commit c1e660f)
Co-authored-by: Peng Ren <ia250@cummins.com> (cherry picked from commit 73d4332)
…nshots `take_tiled_screenshot()` waited for the *absence* of `.loading` elements visible in the viewport before capturing each tile. With DashboardVirtualization on (default), a chart holder that has just scrolled into view but hasn't fired its IntersectionObserver callback yet mounts neither a spinner nor a chart, so the predicate passed vacuously and the tile was captured blank. On top of that, a per-tile timeout was caught, logged as a warning, and the tile was captured anyway -- delivering a spinner screenshot to report recipients instead of failing the report. Replace the absence-of-`.loading` predicate with a positive readiness check: every chart holder (`data-test="dashboard-component-chart-holder"`) intersecting the viewport must show a terminal state (a rendered chart via `.slice_container`, or an error/empty state via `[role="alert"]` / `.ant-empty` / `.missing-chart-container`) before a tile is captured. A holder with nothing mounted no longer satisfies the wait. A per-tile timeout is now logged at ERROR with the tile index, the load_wait, and the identities of the still-unready chart holders, and re-raises instead of being swallowed -- the report now fails (ReportScheduleScreenshotFailedError) instead of silently shipping a degraded screenshot. Co-Authored-By: Claude <noreply@anthropic.com>
…or tile readiness Companion to the previous commit's positive per-tile readiness check, carrying the rest of PR #42119's changes onto this base: - Per-tile readiness timeout logs at WARNING (customer chart-loading issue, not a system fault -- matching #38130/#38441) with elapsed wait time, tile index/total, load_wait, and the identity + stuck-state of each unready chart holder (waiting_on_database / spinner_mounted / nothing_mounted), then re-raises so the report fails rather than shipping a blank or spinner tile. A per-tile DEBUG line logs readiness wait time for profiling. - The deliberate readiness-timeout re-raise is tracked with an explicit flag rather than `except PlaywrightTimeout` at the outer level, since PlaywrightTimeout is aliased to bare Exception when playwright isn't installed and would otherwise swallow-or-propagate the wrong cases. - Threads an optional log_context (e.g. "execution_id=<uuid>") from BaseReportState._get_screenshots through BaseScreenshot.get_screenshot and both WebDriverProxy implementations into take_tiled_screenshot, so timeout logs correlate back to the report run. Defaults to None for callers outside the report pipeline (thumbnails). Backported from #42119 (commits 37da786, f6feb70, 97d1548 squashed) onto this branch's base; only the log lines this change itself adds or touches carry the context suffix -- pre-existing log lines on this base (f-string style) are left as-is. Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
i18n
✅ 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
20 participants
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.
Decisions made that were not in the instructions
aaaa0716734f), whose merge-base withmasteris from 2025-08-18. GitHub therefore renders the entire lineage divergence — ~1,140 commits and ~2,400 changed files — even though this PR's actual payload is only the 2 commits listed below. Do not merge this into master as-is; it exists to run CI on the backported fix. If the intended target is a release branch rather thanmaster, the base can be switched.SUMMARY
Backport of #42119 (tiled-screenshot readiness fix) onto the
elizabeth/report-fixes-42119base. The payload is the top 2 commits only:a9ff21b573— fix(reports): positive per-tile chart readiness check for tiled screenshots. Replaces the absence-of-.loadingper-tile predicate intake_tiled_screenshot()(which passed vacuously when a chart holder scrolled into view before its IntersectionObserver fired, capturing blank charts) with a positive check: every viewport-intersecting[data-test="dashboard-component-chart-holder"]must show a rendered chart (.slice_containerwith no nested.loading) or an error/empty state ([role="alert"],.ant-empty,.missing-chart-container). Clean cherry-pick of the same commit from fix(reports): positive per-tile chart readiness check for tiled screenshots #42119.976e0d9d3e— fix(reports): fail-loud WARNING diagnostics and log-context tracing. On per-tile readiness timeout: log at WARNING (customer chart-loading issue, per fix(screenshots): downgrade screenshot timeout logs from ERROR to WARNING #38130/chore(playwright): Using warning for timeouts #38441 precedent) with elapsed time, tile index,load_wait, and each unready holder's chart id + stuck state (waiting_on_database/spinner_mounted/nothing_mounted), then re-raise so the report fails instead of shipping a spinner/blank tile. Threads optionallog_context(execution_id=<uuid>) fromexecute.pydown totake_tiled_screenshot. The remaining three fix(reports): positive per-tile chart readiness check for tiled screenshots #42119 commits squashed and hand-adapted to this base's older API shapes (get_screenshot(self, url, element_name, user)without defaults, walrus-style call inexecute.py, f-string log style preserved on untouched lines).See #42119 for the full incident background, selector history (#27255/#28745/#39579/#30767/#33107), and design rationale.
TESTING INSTRUCTIONS
On this branch:
tests/unit_tests/utils/test_screenshot_utils.py27/27 pass; webdriver/screenshot/report-execute unit suites 119 pass (1 deselected: pre-existingtest_get_screenshot_handles_playwright_timeoutenvironment artifact, fails identically on the untouched base). Ruff check/format clean on all touched files.ADDITIONAL INFORMATION