eschutho commented on PR #42153: URL: https://github.com/apache/superset/pull/42153#issuecomment-5146254860
Rebased onto current master (clean cherry-pick, no conflicts) and ran an adversarial self-review against the newly merged readiness work before requesting human review. Verdict: no blockers. The scope checks requested for this PR, with evidence: **Production-safe selectors — confirmed.** The diff introduces no `data-test` selectors; it only touches the pre-existing `.chart-container` locator, which is a literal `className` in `Chart.tsx` (the adjacent `data-test="chart-container"` attribute is what `babel-plugin-jsx-remove-data-test-id` strips — the class survives production builds). **Unknown-height/tiling behavior — still useful, and safer than it looks.** By the time the tiling decision runs, `element.wait_for()` has already succeeded, so `dashboard_height == 0` means a transient measurement anomaly on an element known to be visible — exactly the case where routing to `take_tiled_screenshot` (which re-waits and re-measures fresh) is right. Traced the worst cases end-to-end: late layout → tiling proceeds with correct late-measured dimensions; element never re-appears → tiled returns `None` → existing fallback (or, once #42273 lands, a loud failure — consistent with its intent); re-measured height also 0 → `num_tiles = max(1, 0) = 1`, the `clip_height <= 0` guard skips the tile, `combine_screenshot_tiles([])` returns `b""`, which the caller's falsy check handles — and that state requires visible-yet-zero-`scrollHeight`, practically contradictory. **Compatibility with #42624's tiling guard — preserved.** For *known* heights, `use_tiled = likely_large_dashboard and (height_unknown or dashboard_height > tile_height)` still vetoes tiling when `0 < height <= tile_height`, even at high chart counts. Chart count alone bypasses the short-dashboard veto only when height is unknown (0) — so #42624's "chart count alone does not force tiling when the dashboard is shorter than one tile" invariant holds; that PR will only see a textual rebase here, not a semantic conflict. Two review nits applied in the amended commit: - `exc_info=True` on the new chart-container timeout WARNING, for parity with the Selenium locate-wait warnings that all carry it. - A comment noting `locator.all()` is a one-time snapshot, so the rendered/total progress counts describe the snapshot, not the final DOM. One pre-existing inconsistency noted for a follow-up (not this PR): `chart_count` here counts `.chart-container` (mounted charts only), while the merged readiness machinery counts holders via `CHART_HOLDER_SELECTOR` — a strictly better large-dashboard signal since it sees unmounted/virtualized holders too. `tests/unit_tests/utils/` — 715 passed post-amend; `ruff check`/`ruff format --check` clean. -- 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]
