codeant-ai-for-open-source[bot] commented on code in PR #41243: URL: https://github.com/apache/superset/pull/41243#discussion_r3441435639
########## superset/utils/webdriver.py: ########## @@ -117,6 +118,48 @@ def check_playwright_availability() -> bool: PLAYWRIGHT_AVAILABLE = check_playwright_availability() +class _PlaywrightBrowserManager: + """Manages a long-lived Playwright browser instance per worker process. + + In Celery's prefork model, each worker process runs tasks sequentially, + so a single browser instance per process is safe and avoids the overhead + of launching/destroying Chromium on every screenshot task. Each task + creates a lightweight, isolated browser context instead of a full browser. + """ + + def __init__(self) -> None: + self._playwright: Any = None + self._browser: Any = None Review Comment: **Suggestion:** Add an inline docstring to this newly added initializer method to document what internal state is being set up. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The added __init__ method has no docstring. The custom rule requires newly added Python functions and classes to include docstrings, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a053ededb3004bac83acb8716c7da332&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a053ededb3004bac83acb8716c7da332&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/webdriver.py **Line:** 130:132 **Comment:** *Custom Rule: Add an inline docstring to this newly added initializer method to document what internal state is being set up. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=08cde2a868373b4ee80f98c318ad9bd8c61939a482126b67865348852fc6d36d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=08cde2a868373b4ee80f98c318ad9bd8c61939a482126b67865348852fc6d36d&reaction=dislike'>👎</a> ########## superset/utils/webdriver.py: ########## @@ -117,6 +118,48 @@ def check_playwright_availability() -> bool: PLAYWRIGHT_AVAILABLE = check_playwright_availability() +class _PlaywrightBrowserManager: + """Manages a long-lived Playwright browser instance per worker process. + + In Celery's prefork model, each worker process runs tasks sequentially, + so a single browser instance per process is safe and avoids the overhead + of launching/destroying Chromium on every screenshot task. Each task + creates a lightweight, isolated browser context instead of a full browser. + """ + + def __init__(self) -> None: + self._playwright: Any = None + self._browser: Any = None + + def get_browser(self, browser_args: list[str]) -> Any: + """Return a reusable browser, creating one if needed.""" + if self._browser is not None and self._browser.is_connected(): + return self._browser + + self._cleanup() + self._playwright = sync_playwright().start() + self._browser = self._playwright.chromium.launch(args=browser_args) + return self._browser + + def _cleanup(self) -> None: + if self._browser is not None: + try: + self._browser.close() + except Exception: # noqa: S110 + pass + self._browser = None + if self._playwright is not None: + try: + self._playwright.stop() + except Exception: # noqa: S110 + pass + self._playwright = None Review Comment: **Suggestion:** Add an inline docstring to this newly added helper method so its cleanup behavior and intent are explicitly documented. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The newly added _cleanup method does not include a docstring. Since the rule requires newly added Python functions and classes to be documented inline, this is a verified violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f74b5cdada574b3ca2df05dbab7f00e0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f74b5cdada574b3ca2df05dbab7f00e0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/webdriver.py **Line:** 144:156 **Comment:** *Custom Rule: Add an inline docstring to this newly added helper method so its cleanup behavior and intent are explicitly documented. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=b9d1c554a6377fcb58c1321598a83b0dea3e6d0878bd98c750f96c5282620d1d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=b9d1c554a6377fcb58c1321598a83b0dea3e6d0878bd98c750f96c5282620d1d&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/test_playwright_migration_working.py: ########## @@ -98,3 +99,101 @@ def test_webdriver_classes_exist(self): # Should have required attributes assert hasattr(playwright_driver, "_driver_type") assert hasattr(selenium_driver, "_driver_type") + + +class TestPlaywrightBrowserManager: + """Tests for the per-worker browser manager.""" + + def test_initial_state(self): + manager = _PlaywrightBrowserManager() + assert manager._playwright is None + assert manager._browser is None + + def test_get_browser_creates_browser(self): Review Comment: **Suggestion:** Add an explicit return type annotation for this new method (for example, `-> None`) to keep new Python code fully typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This newly added method has no return type annotation. The rule says new Python code should be fully typed, so the missing `-> None` is a verified violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=39c9eb04438e4324814d3bff5b0915dd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=39c9eb04438e4324814d3bff5b0915dd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_playwright_migration_working.py **Line:** 112:112 **Comment:** *Custom Rule: Add an explicit return type annotation for this new method (for example, `-> None`) to keep new Python code fully typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=b1477c729b3dd0736db016fb53d6cf8d3a17e8dd2cc3878c2d15b093bb9da567&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=b1477c729b3dd0736db016fb53d6cf8d3a17e8dd2cc3878c2d15b093bb9da567&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/test_playwright_migration_working.py: ########## @@ -98,3 +99,101 @@ def test_webdriver_classes_exist(self): # Should have required attributes assert hasattr(playwright_driver, "_driver_type") assert hasattr(selenium_driver, "_driver_type") + + +class TestPlaywrightBrowserManager: + """Tests for the per-worker browser manager.""" + + def test_initial_state(self): Review Comment: **Suggestion:** Add an inline docstring to this new test method so newly added functions are documented. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly added Python method and it does not have a docstring. The custom rule requires newly added functions and classes to be documented inline, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2949a5bf37ef412197568a0339130da0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2949a5bf37ef412197568a0339130da0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_playwright_migration_working.py **Line:** 107:107 **Comment:** *Custom Rule: Add an inline docstring to this new test method so newly added functions are documented. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=05f5ea0974e7ebd236454fe3b75be6056525f4145fa90065cdb663d9fe9d59f1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=05f5ea0974e7ebd236454fe3b75be6056525f4145fa90065cdb663d9fe9d59f1&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/test_playwright_migration_working.py: ########## @@ -98,3 +99,101 @@ def test_webdriver_classes_exist(self): # Should have required attributes assert hasattr(playwright_driver, "_driver_type") assert hasattr(selenium_driver, "_driver_type") + + +class TestPlaywrightBrowserManager: + """Tests for the per-worker browser manager.""" + + def test_initial_state(self): + manager = _PlaywrightBrowserManager() + assert manager._playwright is None + assert manager._browser is None + + def test_get_browser_creates_browser(self): + mock_browser = MagicMock() + mock_browser.is_connected.return_value = True + + mock_pw_instance = MagicMock() + mock_pw_instance.chromium.launch.return_value = mock_browser + + mock_sync_pw = MagicMock() + mock_sync_pw.start.return_value = mock_pw_instance + + manager = _PlaywrightBrowserManager() + with patch( + "superset.utils.webdriver.sync_playwright", return_value=mock_sync_pw + ): + browser = manager.get_browser(["--headless"]) + + assert browser is mock_browser + mock_pw_instance.chromium.launch.assert_called_once_with(args=["--headless"]) + + def test_get_browser_reuses_connected_browser(self): Review Comment: **Suggestion:** Add an inline docstring to this newly introduced test method. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new test method has no docstring, which violates the rule that newly added Python functions and classes must include docstrings. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7048c9f302a741fb9e993a62d1b18ce9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7048c9f302a741fb9e993a62d1b18ce9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_playwright_migration_working.py **Line:** 131:131 **Comment:** *Custom Rule: Add an inline docstring to this newly introduced test method. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=7b4d2569a6e24d54c897182ba2ba5607493b58f92b6d69afe83602ba72cec760&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=7b4d2569a6e24d54c897182ba2ba5607493b58f92b6d69afe83602ba72cec760&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/webdriver_test.py: ########## @@ -498,10 +494,10 @@ def test_get_screenshot_works_when_available(self, mock_app, mock_sync_playwrigh ) @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) - @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.logger") def test_get_screenshot_handles_playwright_timeout( - self, mock_logger, mock_sync_playwright + self, mock_logger, mock_browser_manager ): Review Comment: **Suggestion:** Add explicit parameter type annotations and a `-> None` return type to this modified test method to comply with the full-typing requirement for changed Python code. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The changed test method lacks type hints for its parameters and does not declare a return type. Since the surrounding code is being changed, the rule requires full typing here. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9edde1d3c66a4f9f97260a16fd88706d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9edde1d3c66a4f9f97260a16fd88706d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/webdriver_test.py **Line:** 499:501 **Comment:** *Custom Rule: Add explicit parameter type annotations and a `-> None` return type to this modified test method to comply with the full-typing requirement for changed Python code. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=ec7a54d4c9e1426dab5143ae311118479f260ab1593e3a50b2291c2ae578baaa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=ec7a54d4c9e1426dab5143ae311118479f260ab1593e3a50b2291c2ae578baaa&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/webdriver_test.py: ########## @@ -750,10 +734,10 @@ def test_spinner_timeout_logs_warning_and_raises( ) @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) - @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.logger") def test_get_screenshot_raises_on_element_wait_timeout( - self, mock_logger, mock_sync_playwright + self, mock_logger, mock_browser_manager ): Review Comment: **Suggestion:** Provide explicit type annotations for the patched mock parameters and add a `-> None` return annotation in this modified method signature. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This modified test method still has no type annotations for its parameters and no return type. That is a direct violation of the stated rule for changed Python functions. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4a720da9d2964111940b53c9184cdd25&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4a720da9d2964111940b53c9184cdd25&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/webdriver_test.py **Line:** 739:741 **Comment:** *Custom Rule: Provide explicit type annotations for the patched mock parameters and add a `-> None` return annotation in this modified method signature. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=cdafe5c213bff1c4328d5b712706dbd4b2eba3494a2dcf3ceccca5a159e7cbca&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=cdafe5c213bff1c4328d5b712706dbd4b2eba3494a2dcf3ceccca5a159e7cbca&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/webdriver_test.py: ########## @@ -444,9 +444,9 @@ def test_get_screenshot_returns_none_when_unavailable(self, mock_logger, mock_ap assert mock_logger.info.call_args[0][1] == PLAYWRIGHT_INSTALL_MESSAGE @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) - @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.app") - def test_get_screenshot_works_when_available(self, mock_app, mock_sync_playwright): + def test_get_screenshot_works_when_available(self, mock_app, mock_browser_manager): Review Comment: **Suggestion:** Add type hints to the modified test method signature (including patched mock parameters) and declare a `-> None` return type so the changed function is fully typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a modified Python test method and it still omits parameter type hints and a return annotation. The custom rule requires changed Python functions/methods to be fully typed, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7aaf37223c634aa196a4a5198203332e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7aaf37223c634aa196a4a5198203332e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/webdriver_test.py **Line:** 449:449 **Comment:** *Custom Rule: Add type hints to the modified test method signature (including patched mock parameters) and declare a `-> None` return type so the changed function is fully typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=a158259cd591df03fcbe9af1e88dda5a72c730d93cd7848dd04595ddfa77a180&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=a158259cd591df03fcbe9af1e88dda5a72c730d93cd7848dd04595ddfa77a180&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/test_playwright_migration_working.py: ########## @@ -98,3 +99,101 @@ def test_webdriver_classes_exist(self): # Should have required attributes assert hasattr(playwright_driver, "_driver_type") assert hasattr(selenium_driver, "_driver_type") + + +class TestPlaywrightBrowserManager: + """Tests for the per-worker browser manager.""" + + def test_initial_state(self): + manager = _PlaywrightBrowserManager() + assert manager._playwright is None + assert manager._browser is None + + def test_get_browser_creates_browser(self): + mock_browser = MagicMock() + mock_browser.is_connected.return_value = True + + mock_pw_instance = MagicMock() + mock_pw_instance.chromium.launch.return_value = mock_browser + + mock_sync_pw = MagicMock() + mock_sync_pw.start.return_value = mock_pw_instance + + manager = _PlaywrightBrowserManager() + with patch( + "superset.utils.webdriver.sync_playwright", return_value=mock_sync_pw + ): + browser = manager.get_browser(["--headless"]) + + assert browser is mock_browser + mock_pw_instance.chromium.launch.assert_called_once_with(args=["--headless"]) + + def test_get_browser_reuses_connected_browser(self): + mock_browser = MagicMock() + mock_browser.is_connected.return_value = True + + manager = _PlaywrightBrowserManager() + manager._browser = mock_browser + manager._playwright = MagicMock() + + browser = manager.get_browser(["--headless"]) + + assert browser is mock_browser + # Should NOT launch a new browser + manager._playwright.chromium.launch.assert_not_called() + + def test_get_browser_recreates_on_disconnect(self): Review Comment: **Suggestion:** Add an explicit return type annotation for this new method (for example, `-> None`) to satisfy the full-typing rule. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This method is newly introduced and lacks a return annotation. That matches the custom typing rule, so the suggestion is verified. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81491c4d5c584ae6ae17233a6af9e7e7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=81491c4d5c584ae6ae17233a6af9e7e7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_playwright_migration_working.py **Line:** 145:145 **Comment:** *Custom Rule: Add an explicit return type annotation for this new method (for example, `-> None`) to satisfy the full-typing rule. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=446c8b48598e208bc911d0ecd19518856edc6857c488770d4c6268ec95d505b3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=446c8b48598e208bc911d0ecd19518856edc6857c488770d4c6268ec95d505b3&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/webdriver_test.py: ########## @@ -641,10 +633,10 @@ def test_find_unexpected_errors_processes_alerts( mock_close_button.click.assert_called_once() @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) - @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.app") def test_uses_wait_for_function_to_detect_spinners( - self, mock_app, mock_sync_playwright + self, mock_app, mock_browser_manager ): Review Comment: **Suggestion:** Annotate all parameters and the return type in this changed test function signature so newly modified code remains fully typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This changed test method is untyped: both parameters and the return value are missing annotations. That directly matches the custom typing rule for modified Python code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=935109bdf0be4b6b808dd66802148152&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=935109bdf0be4b6b808dd66802148152&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/webdriver_test.py **Line:** 638:640 **Comment:** *Custom Rule: Annotate all parameters and the return type in this changed test function signature so newly modified code remains fully typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=a674acd6d625fa4a07f9d76008c68b66c364bd537b7655ed822ea5b3f90a90db&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=a674acd6d625fa4a07f9d76008c68b66c364bd537b7655ed822ea5b3f90a90db&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/webdriver_test.py: ########## @@ -694,11 +682,11 @@ def test_uses_wait_for_function_to_detect_spinners( assert loading_locator_calls == [] @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) - @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver._browser_manager") @patch("superset.utils.webdriver.logger") @patch("superset.utils.webdriver.app") def test_spinner_timeout_logs_warning_and_raises( - self, mock_app, mock_logger, mock_sync_playwright + self, mock_app, mock_logger, mock_browser_manager ): Review Comment: **Suggestion:** Add type hints for each argument and specify `-> None` for this updated test method to satisfy the typing rule for modified functions. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The updated method is missing both parameter type hints and a return annotation. Because this is modified Python code, the omission violates the full-typing requirement. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6f13f73d49ff409eb8947ef725e5688c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6f13f73d49ff409eb8947ef725e5688c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/webdriver_test.py **Line:** 688:690 **Comment:** *Custom Rule: Add type hints for each argument and specify `-> None` for this updated test method to satisfy the typing rule for modified functions. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=d51a8219b5f8d1e203309565a44338868816bc4c7692fb90a024b0e710166c75&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=d51a8219b5f8d1e203309565a44338868816bc4c7692fb90a024b0e710166c75&reaction=dislike'>👎</a> ########## tests/unit_tests/utils/test_playwright_migration_working.py: ########## @@ -98,3 +99,101 @@ def test_webdriver_classes_exist(self): # Should have required attributes assert hasattr(playwright_driver, "_driver_type") assert hasattr(selenium_driver, "_driver_type") + + +class TestPlaywrightBrowserManager: + """Tests for the per-worker browser manager.""" + + def test_initial_state(self): + manager = _PlaywrightBrowserManager() + assert manager._playwright is None + assert manager._browser is None + + def test_get_browser_creates_browser(self): + mock_browser = MagicMock() + mock_browser.is_connected.return_value = True + + mock_pw_instance = MagicMock() + mock_pw_instance.chromium.launch.return_value = mock_browser + + mock_sync_pw = MagicMock() + mock_sync_pw.start.return_value = mock_pw_instance + + manager = _PlaywrightBrowserManager() + with patch( + "superset.utils.webdriver.sync_playwright", return_value=mock_sync_pw + ): + browser = manager.get_browser(["--headless"]) + + assert browser is mock_browser + mock_pw_instance.chromium.launch.assert_called_once_with(args=["--headless"]) + + def test_get_browser_reuses_connected_browser(self): + mock_browser = MagicMock() + mock_browser.is_connected.return_value = True + + manager = _PlaywrightBrowserManager() + manager._browser = mock_browser + manager._playwright = MagicMock() + + browser = manager.get_browser(["--headless"]) + + assert browser is mock_browser + # Should NOT launch a new browser + manager._playwright.chromium.launch.assert_not_called() + + def test_get_browser_recreates_on_disconnect(self): + stale_browser = MagicMock() + stale_browser.is_connected.return_value = False + + new_browser = MagicMock() + new_browser.is_connected.return_value = True + + mock_pw_instance = MagicMock() + mock_pw_instance.chromium.launch.return_value = new_browser + + mock_sync_pw = MagicMock() + mock_sync_pw.start.return_value = mock_pw_instance + + manager = _PlaywrightBrowserManager() + manager._browser = stale_browser + manager._playwright = MagicMock() + + with patch( + "superset.utils.webdriver.sync_playwright", return_value=mock_sync_pw + ): + browser = manager.get_browser(["--headless"]) + + assert browser is new_browser + stale_browser.close.assert_called_once() + + def test_cleanup(self): + mock_browser = MagicMock() + mock_playwright = MagicMock() + + manager = _PlaywrightBrowserManager() + manager._browser = mock_browser + manager._playwright = mock_playwright + + manager._cleanup() + + mock_browser.close.assert_called_once() + mock_playwright.stop.assert_called_once() + assert manager._browser is None + assert manager._playwright is None + + def test_cleanup_handles_exceptions(self): Review Comment: **Suggestion:** Add a docstring to this new test method so it is documented inline. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This added test method does not include a docstring, so it violates the inline documentation requirement for new functions. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=499b13e21e314ad29518d4ea5b0a8730&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=499b13e21e314ad29518d4ea5b0a8730&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/utils/test_playwright_migration_working.py **Line:** 185:185 **Comment:** *Custom Rule: Add a docstring to this new test method so it is documented inline. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=3f2ec90be5dab6c37bc0b11546c74ad01f3f8b76e69c3eace9748ee55aaf260f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41243&comment_hash=3f2ec90be5dab6c37bc0b11546c74ad01f3f8b76e69c3eace9748ee55aaf260f&reaction=dislike'>👎</a> -- 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]
