Copilot commented on code in PR #36027:
URL: https://github.com/apache/superset/pull/36027#discussion_r2500281479
##########
superset/utils/screenshot_utils.py:
##########
@@ -156,12 +156,28 @@ def take_tiled_screenshot(
remaining_content = dashboard_height - tile_start_in_element
tile_content_height = min(viewport_height, remaining_content)
+ # Determine clip height (use visible element height in viewport)
+ clip_height = min(tile_content_height,
current_element_box["height"])
+
+ # Skip tile if dimensions are invalid (width or height <= 0)
+ # This can happen if element is completely scrolled out of viewport
+ if clip_height <= 0:
Review Comment:
The comment states "Skip tile if dimensions are invalid (width or height <=
0)" but the code only checks `clip_height <= 0`. The width should also be
validated since:
1. The comment explicitly mentions width validation
2. The warning message logs the width value suggesting it should be checked
3. `current_element_box["width"]` could also be <= 0 in edge cases
Add width validation:
```python
if clip_height <= 0 or current_element_box["width"] <= 0:
```
```suggestion
if clip_height <= 0 or current_element_box["width"] <= 0:
```
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -315,3 +315,58 @@ def test_screenshot_clip_parameters(self, mock_page):
assert clip["width"] == 800
# Height should be min of viewport_height and remaining content
assert clip["height"] <= 600 # Element height from mock
+
+ def test_skips_tiles_with_zero_height(self):
+ """Test that tiles with zero height are skipped."""
+ mock_page = MagicMock()
+
+ # Mock element locator
+ element = MagicMock()
+ mock_page.locator.return_value = element
+
+ # Mock element info - 4000px tall dashboard
+ element_info = {"height": 4000, "top": 100, "left": 50, "width": 800}
+
+ # First tile: valid clip region
+ valid_element_box = {"x": 50, "y": 200, "width": 800, "height": 600}
+
+ # Second tile: element scrolled completely out (zero height)
+ invalid_element_box = {"x": 50, "y": -100, "width": 800, "height": 0}
+
+ # For 2 tiles (4000px / 2000px = 2):
+ # 1 initial + 2 scroll + 2 element box + 1 reset = 6 calls
+ mock_page.evaluate.side_effect = [
+ element_info, # Initial call for dashboard dimensions
+ None, # First scroll call
+ valid_element_box, # First element box (valid)
+ None, # Second scroll call
+ invalid_element_box, # Second element box (zero height)
+ None, # Reset scroll call
+ ]
+
+ mock_page.screenshot.return_value = b"fake_screenshot"
+
+ with patch("superset.utils.screenshot_utils.logger") as mock_logger:
+ with patch(
+ "superset.utils.screenshot_utils.combine_screenshot_tiles"
+ ) as mock_combine:
+ mock_combine.return_value = b"combined"
+
+ result = take_tiled_screenshot(
+ mock_page, "dashboard", viewport_height=2000
+ )
+
+ # Should still succeed with partial tiles
+ assert result == b"combined"
+
+ # Should only take 1 screenshot (second tile skipped)
+ assert mock_page.screenshot.call_count == 1
+
+ # Should log warning about skipped tile
+ mock_logger.warning.assert_called_once()
+ warning_call = mock_logger.warning.call_args
+ # Check the format string
Review Comment:
The comment "Check the format string" on line 368 is not entirely accurate.
The assertion checks whether "invalid clip dimensions" appears in the warning
message, which validates the message content rather than the format string
structure. Consider updating to:
```python
# Check the warning message content
assert "invalid clip dimensions" in warning_call[0][0]
```
```suggestion
# Check the warning message content
```
--
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]
