rebenitez1802 commented on PR #43784:
URL: https://github.com/apache/superset/pull/43784#issuecomment-5509066811
**Request changes:** the AG Grid readiness fix and the fail-loud design are
sound and the security model is intact, but the blank-tile detector can
hard-fail an otherwise-deliverable report — that's the one blocking issue.
Solid parts worth calling out: reusing the production-proven
`_agGridFirstDataRendered` contract (correct `undefined → false → true`
lifecycle, so `!== true` really means "not yet painted"), failing loud instead
of shipping a blank PDF, feature-flag safety (exact no-op when
`AG_GRID_TABLE_ENABLED` is off), and the new structured diagnostics.
🔴 **High — Blank-tile detector hard-fails an otherwise-deliverable report**
In `take_tiled_screenshot`, the new guard `is_blank = is_uniform and
visible_chart_holders > 0` rejects any tile that is ≥99.5% one color when the
viewport has ≥1 chart holder, retries 3× against the *same* clip (so identical
pixels), then raises `BlankScreenshotError`, which propagates and fails the
whole report. Two flaws compound: (1) `visible_chart_holders`
(`VISIBLE_CHART_HOLDER_COUNT_JS`) counts holders across the full
`window.innerHeight` viewport, but the capture is only the tile `clip`
sub-region — so a legitimately near-uniform clip gets armed by charts that live
*elsewhere* in the viewport; (2) the 0.995 threshold is content-blind.
Legitimately near-uniform tiles are real: the final partial tile landing on the
~16px dashboard bottom margin/gutter, or a solid-fill deck.gl/mapbox tile (open
ocean / a single landmass — deck.gl isn't covered by the ECharts readiness
gate, so it's declared "rendered"). On `master` these deliver a present PDF;
here they deliver nothi
ng. The last-tile-sliver trigger is a fairly narrow layout class, but the
full-size solid-fill geo-map trigger is broad enough that I'd treat this as
blocking.
Fix: count only holders whose rect intersects `[clip_y, clip_y +
clip_height]` (make the count clip-aware, evaluated *after* the clip is
computed) and exclude terminal empty/error holders; and after exhausting
retries, deliver the tile with a WARNING rather than raising — or only
hard-fail when a repainted frame actually *differs* from the first (a genuine
compositor glitch repaints to non-uniform, whereas static content stays
byte-identical).
🟡 **Medium — Per-capture timeout computed once, reused across all 3 retries**
`capture_timeout` is computed *before* the `for capture_attempt in range(1,
MAX + 1)` loop and reused unchanged, while `_raise_if_budget_exhausted()` only
runs *between* attempts. So the final attempt can be launched with the stale
full ~120s bound even when far less budget remains, letting a wedged compositor
overrun the report deadline by up to one capture window — partially defeating
the constant's own stated goal ("keep each CDP capture bounded so a wedged
compositor cannot consume the report deadline"). Under default config the
overrun window is bounded (roughly the last ~6 min of the 3600s deadline, and
the Celery soft-time-limit is a backstop, so no bad artifact is delivered), so
it's not catastrophic — but the fix is trivial.
Fix: move the `_timeout_seconds("screenshot_capture", requested_seconds=…,
reserve_seconds=…)` call to the top of the loop body so each attempt clamps to
the current remaining budget.
🟡 **Medium — Zero-row AG Grid table can hang readiness, then hard-fail
(narrow, but confirmed mechanism)**
The new `!hasUnpaintedAgGrid` gate holds a holder unready until every
`[data-themed-ag-grid="true"]` host reports `_agGridFirstDataRendered ===
true`. In the shipped `[email protected]`, `firstDataRendered` is
dispatched only per rendered non-pinned data row, so it never fires for an
empty client-side `rowData=[]` — the flag stays `false` and the holder is
pinned in the new `ag_grid_unpainted` state until the readiness budget expires,
then the report fails. This is normally masked because a zero-row table renders
Superset's `.ant-empty` state (which short-circuits via `hasErrorOrEmpty`), but
`ChartRenderer` bypasses the no-results state when `server_pagination &&
(searchText || agGridFilterModel)`, mounting a bare zero-row grid with no
`.ant-empty`. The trigger is narrow (it needs that AG-Grid `ownState` live at
headless-capture time, which the standard report UI doesn't snapshot) and it
fails loud — but the mechanism is confirmed and there's no test for it.
Fix: also set `_agGridFirstDataRendered = true` from `onRowDataUpdated` /
the first `onModelUpdated` in `ThemedAgGridReact` (fires even for zero rows),
and/or treat a host showing `.ag-overlay-no-rows-wrapper` as terminal in the
predicate; add an empty-grid readiness test.
🟢 **Low — Repaint/retry test can't distinguish a discarded blank tile from a
delivered one**
`test_blank_tile_forces_repaint_and_retries` patches
`combine_screenshot_tiles` to a constant and never inspects its args, so it
would still pass if the code appended the blank white tile — the exact
regression this PR exists to prevent. Fix: bind the mock and
`assert_called_once_with([chart_tile])`; optionally assert `blank_tile_retries
== 1` via the emitted log.
🟢 **Low — New `ag_grid_waited_holders` / `blank_tile_retries` diagnostics
are never actually asserted**
In that same test, the `page.evaluate` stub returns `1` for *any* script
containing `getBoundingClientRect`, which also matches
`FIND_CHART_HOLDER_STATES_JS`; the source coerces that non-list to `[]`, so the
final `report_readiness_ready` log is computed over an empty list and the new
aggregations run but are never verified. Fix: dispatch on the exact constant
(`if script == VISIBLE_CHART_HOLDER_COUNT_JS`) and assert the diagnostic values
in the log call.
🟢 **Low — Abort test pins the entire log-context string**
`test_repeated_blank_tile_aborts_report` asserts the full `logger.exception`
suffix (`… report_schedule_id=11 dashboard_id=805 chart_id=None
expected_holders=52 …`), coupling a blank-tile test to
`ReportExecutionContext.log_context` formatting owned in another module — any
field added there breaks this test. Fix: assert the message template plus a
stable substring (or derive the suffix from `ctx.log_context`) rather than
hardcoding the whole rendered string.
🟢 **Low — Capture-timeout path untested, and it silently degrades to `None`**
No test drives `PlaywrightTimeout` from `page.screenshot`, so neither
timeout-then-recover nor timeout-on-all-3 is exercised. Note also an
inconsistency worth confirming: on the final attempt the re-raised
`PlaywrightTimeout` is caught by the generic `except Exception` (which doesn't
set `readiness_timeout`) and returns `None` — so a persistent capture timeout
degrades to `None`, while blank tiles and readiness timeouts fail loudly. Add
both tests; expect `result is None` (not `pytest.raises`) for the all-timeout
case, and confirm the caller treats `None` as a hard failure rather than a
partial delivery.
🟢 **Low — Two unreachable branches**
`is_screenshot_nearly_uniform`'s `if not colors:` guard is dead —
`getcolors(maxcolors=width*height)` can't exceed the pixel count, so it never
returns `None`. And the trailing `if tile_screenshot is None: raise
BlankScreenshotError` after the loop is unreachable for `MAX >= 1`. Both are
harmless. Fix: cap `maxcolors=256` after the thumbnail (that makes the guard
live and treats high-entropy tiles as clearly non-blank), and either drop the
trailing guard or convert it to an `assert
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS >= 1` invariant.
🟢 **Low — UPDATING.md not updated for the new hard-fail mode**
The repo already documents this class of change (reports failing loud
instead of delivering blank output) in `UPDATING.md`, scoped to charts that
never mount (#42624). This PR adds a new *post-readiness* blank-tile hard-fail;
extend that bullet for consistency, since operators who previously received a
(bad) PDF will now see a report error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
