fix: Flakiness around scrolling during taking tiled screenshots with Playwright #36051
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Missing comma in JavaScript object literal ▹ view |
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.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #662b7a
Actionable Suggestions - 1
-
superset/utils/screenshot_utils.py - 1
- Broken tiled screenshot scrolling logic · Line 129-161
Additional Suggestions - 3
-
tests/unit_tests/utils/test_screenshot_utils.py - 3
-
Missing return type annotation for method · Line 177-177Missing return type annotation for `test_logs_dashboard_info` method on line 177. Similar issues exist for methods on lines 204, 236, and 268. Add `-> None` return type annotation.
Code suggestion
@@ -177,1 +177,1 @@ - def test_logs_dashboard_info(self, mock_page): + def test_logs_dashboard_info(self, mock_page) -> None:
-
Nested with statements should be combined · Line 179-179Multiple nested `with` statements on lines 179-180 should be combined into a single `with` statement for better readability. Similar issue exists on lines 243-246.
Code suggestion
@@ -179,2 +179,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"):
-
Commented out code should be removed · Line 232-232Found commented-out code on line 232: `# height=1000 (remaining)`. Consider removing commented code or converting to proper documentation.
Code suggestion
@@ -231,3 +231,2 @@ # Tile 3: y=4100 (dashboard_top + 2*viewport_height) - # height=1000 (remaining) assert screenshot_calls[2][1]["clip"]["y"] == 4100
-
Review Details
-
Files reviewed - 2 · Commit Range:
d2e3853..d2e3853- 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
AI Code Review powered by
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #36051 +/- ##
===========================================
+ Coverage 0 68.71% +68.71%
===========================================
Files 0 622 +622
Lines 0 45752 +45752
Branches 0 4978 +4978
===========================================
+ Hits 0 31438 +31438
- Misses 0 13068 +13068
- Partials 0 1246 +1246
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Antonio-RiveroMartnez
left a comment
Antonio-RiveroMartnez
left a comment
There was a problem hiding this comment.
LGTM, my comments/questions are non-blocking
superset/utils/screenshot_utils.py
Outdated
| logger.debug( | ||
| "Scrolled window to %s for tile %s/%s", scroll_y, i + 1, num_tiles | ||
| ) | ||
| y = dashboard_top + (i * tile_height) |
There was a problem hiding this comment.
nit: I preferred the larger names on these variables, i.e, y and x probably not as readable.
There was a problem hiding this comment.
True, they're x and y coordinates, but it might not be obvious from first glance. I'll change that
superset/utils/screenshot_utils.py
Outdated
| height: rect.height | ||
| }}; | ||
| }}""") | ||
| page.wait_for_timeout(1000) |
There was a problem hiding this comment.
nit: I know this code is like this from before, but should we make that timeout a const at the top? so we can change/play with it later if needed without having o chase it down here?
| combined_screenshot = combine_screenshot_tiles(screenshot_tiles) | ||
|
|
||
| # Reset window scroll position | ||
| page.evaluate("window.scrollTo(0, 0)") |
There was a problem hiding this comment.
is this intended?
There was a problem hiding this comment.
Yes, it wasn't really necessary. scrollTo takes absolute coordinates, not relative, so we don't need to scroll back to the top after each loop
sadpandajoe
added
the
v6.0
github-actions
bot
added
🍒 6.0.0
3 participants
SUMMARY
Supplement of #36027
Simplify the logic around taking tile screenshots with playwright.
This PR removes calculations based on
getBoundingClientRect. Instead, we take tile screenshots by simply calculating the (x,y) coordinates of the clip window based on tile index and viewport height.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Unit tests should pass. For manual testing, create a large dashboard (longer than
SCREENSHOT_TILED_HEIGHT_THRESHOLDfrom config.py) and run a report.ADDITIONAL INFORMATION