bito-code-review[bot] commented on code in PR #36051:
URL: https://github.com/apache/superset/pull/36051#discussion_r2508089975
##########
superset/utils/screenshot_utils.py:
##########
@@ -128,55 +128,35 @@ def take_tiled_screenshot(
for i in range(num_tiles):
# Calculate scroll position to show this tile's content
- scroll_y = dashboard_top + (i * viewport_height)
-
- # Scroll the window to the desired position
- page.evaluate(f"window.scrollTo(0, {scroll_y})")
- logger.debug(
- "Scrolled window to %s for tile %s/%s", scroll_y, i + 1,
num_tiles
- )
-
- # Wait for scroll to settle and content to load
- page.wait_for_timeout(2000) # 2 second wait per tile
-
- # Get the current element position after scroll
- current_element_box = page.evaluate(f"""() => {{
- const el = document.querySelector(".{element_name}");
- const rect = el.getBoundingClientRect();
- return {{
- x: rect.left,
- y: rect.top,
- width: rect.width,
- height: rect.height
- }};
- }}""")
+ y = dashboard_top + (i * viewport_height)
+ x = dashboard_left
# Calculate what portion of the element we want to capture for
this tile
tile_start_in_element = i * viewport_height
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"])
+ clip_height = min(viewport_height, remaining_content)
# 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:
logger.warning(
"Skipping tile %s/%s due to invalid clip dimensions: "
- "width=%s, height=%s (element may be scrolled out of
viewport)",
+ "x=%s, y=%s, width=%s, height=%s "
+ "(element may be scrolled out of viewport)",
i + 1,
num_tiles,
- current_element_box["width"],
+ x,
+ y,
+ dashboard_width,
clip_height,
)
continue
# Clip to capture only the current tile portion of the element
clip = {
- "x": current_element_box["x"],
- "y": current_element_box["y"],
- "width": current_element_box["width"],
+ "x": x,
+ "y": y,
+ "width": dashboard_width,
"height": clip_height,
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken tiled screenshot scrolling logic</b></div>
<div id="fix">
The tiled screenshot logic was modified to remove window scrolling and
dynamic element positioning, which breaks the capture of dashboard tiles
outside the initial viewport. This change prevents proper screenshotting of
large dashboards that require scrolling to access all content. The original
implementation scrolled to each tile position, waited for settling, and
calculated clip coordinates based on the element's current viewport position.
Reverting to include scrolling and position recalculation ensures correct
functionality.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
for i in range(num_tiles):
# Calculate scroll position to show this tile's content
scroll_y = dashboard_top + (i * viewport_height)
# Scroll the window to the desired position
page.evaluate(f"window.scrollTo(0, {scroll_y})")
logger.debug(
"Scrolled window to %s for tile %s/%s", scroll_y, i + 1,
num_tiles
)
# Wait for scroll to settle and content to load
page.wait_for_timeout(2000) # 2 second wait per tile
# Get the current element position after scroll
current_element_box = page.evaluate(f"""() => {{
const el = document.querySelector(".{element_name}");
const rect = el.getBoundingClientRect();
return {{
x: rect.left,
y: rect.top,
width: rect.width,
height: rect.height
}};
}}""")
# Calculate what portion of the element we want to capture for
this tile
tile_start_in_element = i * viewport_height
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:
logger.warning(
"Skipping tile %s/%s due to invalid clip dimensions: "
"width=%s, height=%s (element may be scrolled out of
viewport)",
i + 1,
num_tiles,
current_element_box["width"],
clip_height,
)
continue
# Clip to capture only the current tile portion of the element
clip = {
"x": current_element_box["x"],
"y": current_element_box["y"],
"width": current_element_box["width"],
"height": clip_height,
}
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36051#issuecomment-3508401435>#662b7a</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
