bito-code-review[bot] commented on code in PR #36126:
URL: https://github.com/apache/superset/pull/36126#discussion_r2529486076
##########
superset/utils/screenshots.py:
##########
@@ -75,67 +218,42 @@ def cache_key(
}
return md5_sha_from_dict(args)
- def get_screenshot(
- self, user: User, window_size: WindowSize | None = None
- ) -> bytes | None:
- driver = self.driver(window_size)
- self.screenshot = driver.get_screenshot(self.url, self.element, user)
- return self.screenshot
-
- def get(
- self,
- user: User = None,
- cache: Cache = None,
- thumb_size: WindowSize | None = None,
- ) -> BytesIO | None:
- """
- Get thumbnail screenshot has BytesIO from cache or fetch
-
- :param user: None to use current user or User Model to login and fetch
- :param cache: The cache to use
- :param thumb_size: Override thumbnail site
- """
- payload: bytes | None = None
- cache_key = self.cache_key(self.window_size, thumb_size)
- if cache:
- payload = cache.get(cache_key)
- if not payload:
- payload = self.compute_and_cache(
- user=user, thumb_size=thumb_size, cache=cache
- )
- else:
- logger.info("Loaded thumbnail from cache: %s", cache_key)
- if payload:
- return BytesIO(payload)
- return None
-
def get_from_cache(
self,
- cache: Cache,
window_size: WindowSize | None = None,
thumb_size: WindowSize | None = None,
- ) -> BytesIO | None:
- cache_key = self.cache_key(window_size, thumb_size)
- return self.get_from_cache_key(cache, cache_key)
+ ) -> ScreenshotCachePayload | None:
+ cache_key = self.get_cache_key(window_size, thumb_size)
+ return self.get_from_cache_key(cache_key)
- @staticmethod
- def get_from_cache_key(cache: Cache, cache_key: str) -> BytesIO | None:
+ @classmethod
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>API compatibility broken by method signature
change</b></div>
<div id="fix">
Changing get_from_cache_key from a staticmethod taking (cache, cache_key) to
a classmethod taking only (cache_key) breaks the existing call in
superset/charts/api.py that passes thumbnail_cache and digest as arguments.
This will cause a TypeError at runtime when the chart thumbnail endpoint is
accessed. The fix updates the call to pass only the digest and handles the new
ScreenshotCachePayload return type by extracting the BytesIO image.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- if img := ChartScreenshot.get_from_cache_key(thumbnail_cache,
digest):
- return Response(
- FileWrapper(img), mimetype="image/png",
direct_passthrough=True
+ if img := ChartScreenshot.get_from_cache_key(digest):
+ img = img.get_image()
+ return Response(
+ FileWrapper(img), mimetype="image/png",
direct_passthrough=True
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36126#issuecomment-3535451499>#cf50c2</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
##########
superset/utils/screenshots.py:
##########
@@ -16,23 +16,48 @@
# under the License.
from __future__ import annotations
+import base64
import logging
+from datetime import datetime
+from enum import Enum
from io import BytesIO
-from typing import TYPE_CHECKING
+from typing import cast, TYPE_CHECKING, TypedDict
-from flask import current_app
+from flask import current_app as app
+from superset import feature_flag_manager, thumbnail_cache
+from superset.exceptions import ScreenshotImageNotAvailableException
+from superset.extensions import event_logger
from superset.utils.hashing import md5_sha_from_dict
from superset.utils.urls import modify_url_query
from superset.utils.webdriver import (
ChartStandaloneMode,
DashboardStandaloneMode,
- WebDriverProxy,
+ WebDriver,
+ WebDriverPlaywright,
+ WebDriverSelenium,
WindowSize,
)
logger = logging.getLogger(__name__)
+# Import Playwright availability and install message
+try:
+ from superset.utils.webdriver import (
+ PLAYWRIGHT_AVAILABLE,
+ PLAYWRIGHT_INSTALL_MESSAGE,
+ )
+except ImportError:
+ PLAYWRIGHT_AVAILABLE = False
+ PLAYWRIGHT_INSTALL_MESSAGE = "Playwright module not found"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Playwright constants from webdriver
module</b></div>
<div id="fix">
The constants `PLAYWRIGHT_AVAILABLE` and `PLAYWRIGHT_INSTALL_MESSAGE` are
imported from `superset.utils.webdriver` but these attributes don't exist in
the module. The code has fallback handling in the except block, but the import
will still fail. Consider moving the import inside the try block.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
# Import Playwright availability and install message
try:
from superset.utils.webdriver import (
PLAYWRIGHT_AVAILABLE,
PLAYWRIGHT_INSTALL_MESSAGE,
)
except (ImportError, AttributeError):
PLAYWRIGHT_AVAILABLE = False
PLAYWRIGHT_INSTALL_MESSAGE = "Playwright module not found"
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36126#issuecomment-3535451499>#cf50c2</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
##########
superset/utils/screenshots.py:
##########
@@ -43,23 +68,141 @@
from flask_caching import Cache
+class StatusValues(Enum):
+ PENDING = "Pending"
+ COMPUTING = "Computing"
+ UPDATED = "Updated"
+ ERROR = "Error"
+
+
+class ScreenshotCachePayloadType(TypedDict):
+ image: str | None
+ timestamp: str
+ status: str
+
+
+class ScreenshotCachePayload:
+ def __init__(
+ self,
+ image: bytes | None = None,
+ status: StatusValues = StatusValues.PENDING,
+ timestamp: str = "",
+ ):
+ self._image = image
+ self._timestamp = timestamp or datetime.now().isoformat()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Datetime now called without timezone</b></div>
<div id="fix">
Multiple `datetime.now()` calls without timezone argument on lines 92, 113,
and 150. Consider using `datetime.now(timezone.utc)` for timezone-aware
timestamps.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
self._image = image
self._timestamp = timestamp or datetime.now(timezone.utc).isoformat()
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36126#issuecomment-3535451499>#cf50c2</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]
