madhushreeag commented on PR #43028: URL: https://github.com/apache/superset/pull/43028#issuecomment-5256805272
> ### Code Review Agent Run #ecced6 > Actionable Suggestions - 0 > Additional Suggestions - 4 > * superset/mcp_service/screenshot/pooled_screenshot.py - 1 > > * Semantic duplication with ChartScreenshot constructor · [Line 61-84](https://github.com/apache/superset/pull/43028/files#diff-f0d8a65e88da09e125b5fc99664b11c19df071a11336b2c49d0d6a42ef998290L61) > PooledChartScreenshot (lines 61-84) and ChartScreenshot (screenshots.py lines 432-446) implement nearly identical constructors: same url modification with ChartStandaloneMode.HIDE_NAV, same window_size default (800, 600). This semantic duplication creates divergence risk. > * superset/utils/screenshots.py - 2 > > * WebDriverSelenium class may be dead code · [Line 28-57](https://github.com/apache/superset/pull/43028/files#diff-5a9ca4cc22570c299d9cd1f5defa1414900451594dbb2998c7e2eba29285c805L57) > The diff removes WebDriverSelenium from the screenshot workflow. Verify if the WebDriverSelenium class definition itself is now dead code that should be removed, or if it's still used elsewhere in the codebase. > * Empty driver_type passed to Playwright · [Line 210-210](https://github.com/apache/superset/pull/43028/files#diff-5a9ca4cc22570c299d9cd1f5defa1414900451594dbb2998c7e2eba29285c805R210) > The `driver_type` property was removed but empty string `''` is passed to `WebDriverPlaywright`. Verify this is intentional — if `_driver_type` is truly unused in the Playwright path, passing an empty string is safe but should be documented. > * tests/unit_tests/utils/webdriver_test.py - 1 > > * Test assertion mismatch · [Line 115-121](https://github.com/apache/superset/pull/43028/files#diff-1c062e18bcc063198dad28bf247e41fb603b3f710c1670c4fdd54b88356c0c91R115) > Test `test_check_playwright_availability_ignores_runtime_errors` has misleading comment 'Even if the mock raises' but mock is not configured to raise. Since the implementation at line 83 only checks `sync_playwright is not None` (no function invocation), this test passes vacuously without validating the stated error-handling behavior. > > Filtered by Review Rules > Review Details > Bito Usage Guide > AI Code Review powered by [<img alt="Bito Logo" width="50" height="20" src="https://camo.githubusercontent.com/810f9c950a15f18a9a21d4e214820ef360ea3b08a4b5dea95d24529fffd09ed6/68747470733a2f2f6269746f2e61692f77702d636f6e74656e742f75706c6f6164732f323032332f31302f4c6f676f2d4269746f2d426c61636b2d63726f707065642e737667";>](https://bito.ai/) WebDriverSelenium dead code suggestion is a false alarm, we deleted that class in this PR Addressed the other suggestions. -- 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]
