bito-code-review[bot] commented on code in PR #43762:
URL: https://github.com/apache/superset/pull/43762#discussion_r3904724957
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -279,11 +272,7 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
);
return (
- <ChartHeaderStyles
- className="slice-header"
- data-test="slice-header"
- ref={ref}
- >
+ <ChartHeaderStyles data-test="slice-header" ref={ref}>
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Removed CSS class breaks test</b></div>
<div id="fix">
Removing `className="slice-header"` breaks `SliceHeader.test.tsx:232`
(`toHaveClass('slice-header')`) and the fullscreen z-index rule in
`SliceHeaderControls/Styles.tsx:55`
(`.dashboard-component-chart-holder:fullscreen .slice-header`). Restore the
class on `ChartHeaderStyles`.
</div>
</div>
<small><i>Code Review Run #5a16fc</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-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -721,15 +724,12 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
validator_config_json: {},
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken tests from removed feature</b></div>
<div id="fix">
This diff removes the `include_cta` checkbox UI, its default value, and the
submit payload, but `AlertReportModal.test.tsx` (unchanged) still has 5 tests
asserting on the 'include a link back to superset' checkbox and
`body.include_cta` (lines 737-820, 1642-1689). These tests will fail in CI.
Update or remove them to match the removed feature.
</div>
</div>
<small><i>Code Review Run #5a16fc</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:
##########
@@ -196,35 +188,21 @@ def is_computing_stale(self) -> bool:
datetime.now() - datetime.fromisoformat(self.get_timestamp())
).total_seconds() >= computing_ttl
- def should_trigger_task(
- self, force: bool = False, expected_scope: str | None = None
- ) -> bool:
- """
- :param expected_scope: The scope (e.g. "dashboard:<id>") the caller
- requires this entry to carry. Entries written before scope
- tracking existed -- or by a stale/mismatched caller -- deserialize
- with no scope (or a different one) and are otherwise
- indistinguishable from a fresh, valid ``UPDATED`` entry, which
- would leave them permanently un-refreshed: the scope check at
- read time rejects them, but nothing ever re-triggers computation.
- Treat a scope mismatch on an ``UPDATED`` entry as a cache miss so
- it gets recomputed and re-scoped.
- """
+ def should_trigger_task(self, force: bool = False) -> bool:
return (
force
or self.status == StatusValues.PENDING
or (self.status == StatusValues.ERROR and
self.is_error_cache_ttl_expired())
or (self.status == StatusValues.COMPUTING and
self.is_computing_stale())
or (self.status == StatusValues.UPDATED and self._image is None)
- or (
- self.status == StatusValues.UPDATED
- and expected_scope is not None
- and self._scope != expected_scope
- )
)
class BaseScreenshot:
+ @property
+ def driver_type(self) -> str:
+ return app.config["WEBDRIVER_TYPE"]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing config key KeyError</b></div>
<div id="fix">
`driver_type` reads `app.config["WEBDRIVER_TYPE"]`, but `WEBDRIVER_TYPE` is
not defined in `superset/config.py` (only referenced in tests/README). This
raises `KeyError` on every screenshot call. Use
`app.config.get("WEBDRIVER_TYPE", "chrome")` or add the key to config.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
def driver_type(self) -> str:
return app.config.get("WEBDRIVER_TYPE", "chrome")
````
</div>
</details>
</div>
<small><i>Code Review Run #5a16fc</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]
