bito-code-review[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3250798343
##########
superset-frontend/src/extensions/ExtensionsStartup.test.tsx:
##########
@@ -259,38 +251,36 @@ test('does not initialize ExtensionsLoader when
EnableExtensions feature flag is
initializeSpy.mockRestore();
});
-test('logs error when ExtensionsLoader initialization fails', async () => {
+test('continues rendering children even when ExtensionsLoader initialization
fails', async () => {
// Ensure feature flag is enabled
mockIsFeatureEnabled.mockReturnValue(true);
- const errorSpy = jest.spyOn(logging, 'error').mockImplementation();
-
- // Mock the initializeExtensions method to throw an error
+ // Mock the initializeExtensions method to reject — ExtensionsLoader handles
+ // its own error logging internally
const originalInitialize = ExtensionsLoader.prototype.initializeExtensions;
ExtensionsLoader.prototype.initializeExtensions = jest
.fn()
- .mockImplementation(() => {
- throw new Error('Test initialization error');
- });
+ .mockImplementation(() => Promise.resolve());
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mock setup contradicts test name</b></div>
<div id="fix">
Test name says 'when ExtensionsLoader initialization fails' and comment
(line 258-259) says 'Mock the initializeExtensions method to reject', yet the
mock uses `Promise.resolve()` instead of `Promise.reject()`. This test does not
actually test the failure scenario it claims to test. The mock must resolve to
an error for the test assertions to be meaningful.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
+++ b/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
@@ -260,7 +260,7 @@ describe('ExtensionsStartup', () => {
ExtensionsLoader.prototype.initializeExtensions = jest
.fn()
- .mockImplementation(() => Promise.resolve());
+ .mockRejectedValue(new Error('Test initialization error'));
const { container } = render(
<ExtensionsStartup>
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</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
##########
docs/versioned_docs/version-6.0.0/faq.mdx:
##########
@@ -51,11 +51,11 @@ multiple tables as long as your database account has access
to the tables.
## How do I create my own visualization?
We recommend reading the instructions in
-[Creating Visualization
Plugins](/docs/6.0.0/contributing/howtos#creating-visualization-plugins).
+[Creating Visualization
Plugins](/user-docs/6.0.0/contributing/howtos#creating-visualization-plugins).
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken link to wrong doc section</b></div>
<div id="fix">
The 'Creating Visualization Plugins' section is located in
`developer_docs/contributing/howtos.md` (line 58), not in user-docs. Users
clicking this link will encounter a 404 or incorrect page. The correct path is
`/developer-docs/contributing/howtos#creating-visualization-plugins`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- docs/versioned_docs/version-6.0.0/faq.mdx
+++ docs/versioned_docs/version-6.0.0/faq.mdx
@@ -51,7 +51,7 @@
## How do I create my own visualization?
We recommend reading the instructions in
-[Creating Visualization
Plugins](/user-docs/6.0.0/contributing/howtos#creating-visualization-plugins).
+[Creating Visualization
Plugins](/developer-docs/contributing/howtos#creating-visualization-plugins).
## Can I upload and visualize CSV data?
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</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/chart/tool/get_chart_sql.py:
##########
@@ -32,6 +32,13 @@
from superset.commands.explore.form_data.parameters import CommandParameters
from superset.exceptions import SupersetException, SupersetSecurityException
from superset.extensions import event_logger
+from superset.mcp_service.chart.chart_helpers import (
+ build_query_context_from_form_data,
+ extract_x_axis_col,
+ resolve_groupby,
+ resolve_metrics,
+ resolve_metrics_and_groupby,
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused imports and wrappers</b></div>
<div id="fix">
Four imported functions from `chart_helpers` are unused:
`extract_x_axis_col`, `resolve_groupby`, `resolve_metrics`, and
`resolve_metrics_and_groupby`. Their wrapper functions are defined but never
called elsewhere in the module. This dead code increases maintenance burden and
could confuse future contributors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/mcp_service/chart/tool/get_chart_sql.py
+++ b/superset/mcp_service/chart/tool/get_chart_sql.py
@@ -32,13 +32,7 @@ from superset.commands.explore.form_data.parameters
import CommandParameters
from superset.exceptions import SupersetException,
SupersetSecurityException
from superset.extensions import event_logger
from superset.mcp_service.chart.chart_helpers import (
build_query_context_from_form_data,
- extract_x_axis_col,
- resolve_groupby,
- resolve_metrics,
- resolve_metrics_and_groupby,
)
from superset.mcp_service.chart.chart_utils import validate_chart_dataset
from superset.mcp_service.chart.schemas import (
@@ -77,23 +71,6 @@ def _sanitize_chart_sql_for_llm_context(chart_sql:
ChartSql) -> ChartSql:
return None
-def _resolve_metrics(form_data: dict[str, Any], viz_type: str) ->
list[Any]:
- """Extract metrics from form_data, handling chart-type-specific
fields."""
- return resolve_metrics(form_data, viz_type)
-
-
-def _resolve_groupby(form_data: dict[str, Any]) -> list[Any]:
- """Extract groupby columns from form_data with fallback aliases."""
- return resolve_groupby(form_data)
-
-
def _resolve_metrics_and_groupby(
- form_data: dict[str, Any],
- chart: "Slice | None",
-) -> tuple[list[Any], list[Any]]:
- """Resolve metrics and groupby columns from form_data."""
- return resolve_metrics_and_groupby(form_data, chart)
-
-
def _extract_x_axis_col(form_data: dict[str, Any]) -> str | None:
- """Return the x_axis column name from form_data, or None if not set."""
- return extract_x_axis_col(form_data)
-
-
def _build_query_context_from_form_data(
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</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
##########
docs/versioned_docs/version-6.0.0/contributing/guidelines.mdx:
##########
@@ -57,7 +57,7 @@ Finally, never submit a PR that will put master branch in
broken state. If the P
in `requirements.txt` pinned to a specific version which ensures that the
application
build is deterministic.
- For TypeScript/JavaScript, include new libraries in `package.json`
-- **Tests:** The pull request should include tests, either as doctests, unit
tests, or both. Make sure to resolve all errors and test failures. See
[Testing](/docs/6.0.0/contributing/howtos#testing) for how to run tests.
+- **Tests:** The pull request should include tests, either as doctests, unit
tests, or both. Make sure to resolve all errors and test failures. See
[Testing](/user-docs/6.0.0/contributing/howtos#testing) for how to run tests.
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken documentation link</b></div>
<div id="fix">
Update the test documentation link to use a relative path to the local
howtos file. For example:
```md
See [Testing](howtos#testing) for how to run tests.
```
</div>
</div>
<small><i>Code Review Run #a63702</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]
