bito-code-review[bot] commented on code in PR #38689:
URL: https://github.com/apache/superset/pull/38689#discussion_r2954714154
##########
superset/mcp_service/screenshot/webdriver_pool.py:
##########
@@ -115,70 +115,36 @@ def get_stats(self) -> Dict[str, Any]:
"max_pool_size": self.max_pool_size,
}
- def _create_driver(
- self, window_size: WindowSize, user_id: int | None = None
- ) -> PooledWebDriver:
- """Create a new WebDriver instance with timeout protection"""
- driver = None
- old_handler = None
-
+ def _create_driver(self) -> PooledWebDriver:
+ """Create a new webdriver instance with timeout protection"""
try:
# SECURITY FIX: Set up timeout protection for driver creation
- old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
- signal.alarm(self.creation_timeout_seconds)
+ old_handler = None
+ if platform.system() != "Windows":
+ old_handler = signal.signal(signal.SIGALRM,
self._timeout_handler)
+ signal.alarm(self.creation_timeout_seconds)
driver_type = current_app.config.get("WEBDRIVER_TYPE", "firefox")
- selenium_driver = WebDriverSelenium(driver_type, window_size)
-
- # Create the actual WebDriver with timeout protection
- driver = selenium_driver.create()
+ driver = WebDriverSelenium(driver_type, window_size)
driver.set_window_size(*window_size)
# Clear the alarm - creation successful
- signal.alarm(0)
+ if platform.system() != "Windows":
+ signal.alarm(0)
pooled_driver = PooledWebDriver(
driver=driver,
- created_at=time.time(),
- last_used=time.time(),
- window_size=window_size,
- user_id=user_id,
- is_healthy=True,
- usage_count=0,
- )
-
- self._stats["created"] += 1
- logger.debug(
- "Created new WebDriver instance for window size %s",
window_size
+ in_use=False,
+ last_used=datetime.now()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type mismatch</b></div>
<div id="fix">
last_used expects float but gets datetime object, causing TypeError. Use
time.time().
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
last_used=time.time()
````
</div>
</details>
</div>
<small><i>Code Review Run #a8ee2c</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
##########
superset/mcp_service/screenshot/webdriver_pool.py:
##########
@@ -115,70 +115,36 @@ def get_stats(self) -> Dict[str, Any]:
"max_pool_size": self.max_pool_size,
}
- def _create_driver(
- self, window_size: WindowSize, user_id: int | None = None
- ) -> PooledWebDriver:
- """Create a new WebDriver instance with timeout protection"""
- driver = None
- old_handler = None
-
+ def _create_driver(self) -> PooledWebDriver:
+ """Create a new webdriver instance with timeout protection"""
try:
# SECURITY FIX: Set up timeout protection for driver creation
- old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
- signal.alarm(self.creation_timeout_seconds)
+ old_handler = None
+ if platform.system() != "Windows":
+ old_handler = signal.signal(signal.SIGALRM,
self._timeout_handler)
+ signal.alarm(self.creation_timeout_seconds)
driver_type = current_app.config.get("WEBDRIVER_TYPE", "firefox")
- selenium_driver = WebDriverSelenium(driver_type, window_size)
-
- # Create the actual WebDriver with timeout protection
- driver = selenium_driver.create()
+ driver = WebDriverSelenium(driver_type, window_size)
driver.set_window_size(*window_size)
# Clear the alarm - creation successful
- signal.alarm(0)
+ if platform.system() != "Windows":
+ signal.alarm(0)
pooled_driver = PooledWebDriver(
driver=driver,
- created_at=time.time(),
- last_used=time.time(),
- window_size=window_size,
- user_id=user_id,
- is_healthy=True,
- usage_count=0,
- )
-
- self._stats["created"] += 1
- logger.debug(
- "Created new WebDriver instance for window size %s",
window_size
+ in_use=False,
+ last_used=datetime.now()
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undefined datetime and missing fields</b></div>
<div id="fix">
Undefined name `datetime` and missing required `PooledWebDriver` fields.
Import `datetime` from datetime module and add all required fields:
`created_at`, `last_used`, `window_size`, `user_id`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
pooled_driver = PooledWebDriver(
driver=driver,
created_at=time.time(),
last_used=time.time(),
window_size=window_size,
user_id=user_id,
)
````
</div>
</details>
</div>
<small><i>Code Review Run #a8ee2c</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
##########
superset/mcp_service/screenshot/webdriver_pool.py:
##########
@@ -115,70 +115,36 @@ def get_stats(self) -> Dict[str, Any]:
"max_pool_size": self.max_pool_size,
}
- def _create_driver(
- self, window_size: WindowSize, user_id: int | None = None
- ) -> PooledWebDriver:
- """Create a new WebDriver instance with timeout protection"""
- driver = None
- old_handler = None
-
+ def _create_driver(self) -> PooledWebDriver:
+ """Create a new webdriver instance with timeout protection"""
try:
# SECURITY FIX: Set up timeout protection for driver creation
- old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
- signal.alarm(self.creation_timeout_seconds)
+ old_handler = None
+ if platform.system() != "Windows":
+ old_handler = signal.signal(signal.SIGALRM,
self._timeout_handler)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Wrong handler reference</b></div>
<div id="fix">
self._timeout_handler doesn't exist; use module-level _timeout_handler.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
old_handler = signal.signal(signal.SIGALRM, _timeout_handler)
````
</div>
</details>
</div>
<small><i>Code Review Run #a8ee2c</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]
