fix(playwright): Fix tiled screenshot clip bounds validation

[eschutho]

SUMMARY

Fixes the "Clipped area is either empty or outside the resulting image" error that occurs when generating tiled screenshots for large dashboards (50+ charts).

Root Cause:
When scrolling through tall dashboards to capture tiles, getBoundingClientRect() returns negative Y coordinates for elements scrolled above the viewport. Playwright rejects these invalid clip coordinates, causing the screenshot to fail.

Solution:

• Add comprehensive viewport bounds tracking and validation
• Clamp clip coordinates to always be within viewport (≥0)
• Calculate visible portions when elements are partially scrolled off-screen
• Skip invalid tiles with warnings instead of crashing
• Validate all clip dimensions before calling page.screenshot()

Changes:

• superset/utils/screenshot_utils.py: Enhanced clip validation logic (lines 142-205)
• tests/unit_tests/utils/test_screenshot_utils.py: Added 6 new edge case tests

Test Coverage:
All 21 unit tests passing, including new tests for:

• Negative element positions (scrolled above viewport)
• Elements extending beyond viewport boundaries
• Invalid clip dimensions (zero/negative width/height)
• Elements completely scrolled out of view
• Zero-width elements

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

playwright._impl._errors.Error: Page.screenshot: Clipped area is either empty or outside the resulting image
Call log:
taking page screenshot
  - waiting for fonts to load...
  - fonts loaded

Tiled screenshot fails → Falls back to single screenshot → Empty PDF for large dashboards

AFTER:

• Tiled screenshots complete successfully for dashboards with 50+ charts
• No more clip validation errors
• Invalid tiles are gracefully skipped with warnings

TESTING INSTRUCTIONS

1. Unit Tests:

   pytest tests/unit_tests/utils/test_screenshot_utils.py -v

   All 21 tests should pass

2. Manual Testing (requires large dashboard):

   • Create a dashboard with 50+ charts (one per row)
   • Enable tiled screenshots:

     SCREENSHOT_TILED_ENABLED = True
     SCREENSHOT_TILED_CHART_THRESHOLD = 20

   • Generate a screenshot/report for the dashboard
   • Verify: No "Clipped area" errors in logs
   • Verify: PDF/image is generated correctly (not empty)

3. Verify Logs:

   • Should see "Large dashboard detected" and "Using tiled screenshots" messages
   • Should NOT see clip validation errors
   • May see "Skipping tile X/Y - invalid clip dimensions" warnings (expected for edge cases)

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags: SCREENSHOT_TILED_ENABLED=True (optional, for testing)
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[bito-code-review]

Code Review Agent Run #b6d49e

Actionable Suggestions - 0

Additional Suggestions - 4

• superset/utils/screenshot_utils.py - 1

  • Missing trailing comma in function call · Line 179-179
    Missing trailing comma after `viewport_info["viewportWidth"] - clip_x` on line 179. Adding a trailing comma improves code consistency and makes future modifications easier.
    Code suggestion

     @@ -178,2 +178,2 @@
                 clip_width = min(
    -                viewport_info["elementWidth"], viewport_info["viewportWidth"] - clip_x
    +                viewport_info["elementWidth"], viewport_info["viewportWidth"] - clip_x,
                 )

• tests/unit_tests/utils/test_screenshot_utils.py - 3

  • Missing return type annotation for test method · Line 347-347
    Test method `test_negative_element_position_clipped_to_zero` is missing return type annotation. This issue occurs in 5 other test methods in this file. Consider adding `-> None` return type annotation for consistency.
    Code suggestion

     @@ -347,1 +347,1 @@
    -    def test_negative_element_position_clipped_to_zero(self):
    +    def test_negative_element_position_clipped_to_zero(self) -> None:

  • Nested with statements should be combined · Line 468-468
    Multiple nested `with` statements can be combined into a single statement for better readability. This pattern occurs in 3 locations in this file. Use `with patch(...), patch(...):` syntax instead.
    Code suggestion

     @@ -468,2 +468,1 @@
    -        with patch("superset.utils.screenshot_utils.logger") as mock_logger:
    -            with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
    +        with patch("superset.utils.screenshot_utils.logger") as mock_logger, patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):

  • Missing trailing comma in function call · Line 471-471
    Missing trailing comma after `viewport_height=2000` parameter. This issue also occurs on lines 553 and 596. Adding trailing commas improves consistency and makes future additions easier.
    Code suggestion

     @@ -470,2 +470,2 @@
    -                result = take_tiled_screenshot(
    -                    mock_page, "dashboard", viewport_height=2000
    +                result = take_tiled_screenshot(
    +                    mock_page, "dashboard", viewport_height=2000,

Review Details

• Files reviewed - 2 · Commit Range: 782f5ea..782f5ea

  • superset/utils/screenshot_utils.py
  • tests/unit_tests/utils/test_screenshot_utils.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/utils/screenshot_utils.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[bito-code-review]

Interaction Diagram by Bito

sequenceDiagram
participant REP as Report Execute Command
participant SCR as Screenshot Service<br/>🔄 Updated | ●●○ Medium
participant WEB as WebDriver Playwright
participant TSS as take_tiled_screenshot<br/>🔄 Updated | ●●● High
participant PAGE as Playwright Page
participant CMB as combine_screenshot_tiles
Note over TSS: Enhanced viewport bounds<br/>validation & clipping logic
REP->>SCR: get_screenshot(user)
SCR->>WEB: get_screenshot(url, element, user)
WEB->>TSS: take_tiled_screenshot(page, element_name)
TSS->>PAGE: evaluate() - get viewport info
PAGE-->>TSS: viewport dimensions & element bounds
TSS->>PAGE: screenshot(clip) - with validated bounds
PAGE-->>TSS: tile screenshot bytes
TSS->>CMB: combine_screenshot_tiles(tiles)
CMB-->>TSS: combined screenshot
TSS-->>WEB: screenshot bytes
WEB-->>SCR: screenshot data
SCR-->>REP: final screenshot

Loading

Critical path: Report Execute Command->Screenshot Service->WebDriver Playwright->take_tiled_screenshot->combine_screenshot_tiles

Note: The take_tiled_screenshot function now includes robust viewport bounds validation and clipping logic to handle edge cases like negative element positions and elements extending beyond viewport boundaries. This prevents invalid clip dimensions and improves screenshot reliability for large dashboards.

[sadpandajoe]

From codex, we should see if the test will catch the regression or not.

The configured SCREENSHOT_TILED_VIEWPORT_HEIGHT (default 2000) onto the scroll math, while Playwright’s browser window stays around 1200px tall. With your new clipping logic, Playwright no longer throws, but we quietly skip content after the first tile (0–1200, then we jump to 2000–3200, leaving ~800px unaccounted for). To fix both the crash and the coverage, we can do the following:

• Fetch the actual viewport height once (actual_height = page.viewport_size["height"] or via window.innerHeight) and set tile_height = min(viewport_height, actual_height).
• Use tile_height for both num_tiles and the scroll increment instead of the configured viewport_height.
• Keep the original clip dictionary (with a quick clip_y = max(0, clip_y) guard if needed), so we still capture the full element per tile.

Here’s a regression test you can drop under TestTakeTiledScreenshot to lock the behavior down:

    def test_scroll_increment_respects_actual_viewport_height(self):
          """When config viewport height > actual viewport, we still cover every tile."""
          mock_page = MagicMock()
          mock_page.viewport_size = {"width": 1600, "height": 1200}

          element = MagicMock()
          mock_page.locator.return_value = element

          element_info = {"height": 3600, "top": 0, "left": 0, "width": 800}
          element_box = {"x": 0, "y": 0, "width": 800, "height": 1200}

          mock_page.evaluate.side_effect = [
              element_info,
              None, element_box,       # first tile
              None, element_box,       # second tile
              None, element_box,       # third tile
              None,                    # reset scroll
          ]

          mock_page.screenshot.return_value = b"screenshot"

          with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"):
              take_tiled_screenshot(mock_page, "dashboard", viewport_height=2000)

          # We expect three tiles (0–1200, 1200–2400, 2400–3600) even though config says 2000.
          assert mock_page.screenshot.call_count == 3

          scroll_calls = [
              call.args[0]
              for call in mock_page.evaluate.call_args_list
              if call.args[0].startswith("window.scrollTo")
          ]
          assert scroll_calls == [
              "window.scrollTo(0, 0)",
              "window.scrollTo(0, 1200)",
              "window.scrollTo(0, 2400)",
          ]

[eschutho]

I'm handing this off to @kgabryje