bito-code-review[bot] commented on code in PR #34764:
URL: https://github.com/apache/superset/pull/34764#discussion_r2317573296
##########
superset/themes/schemas.py:
##########
@@ -45,20 +42,24 @@ def validate_json_data(self, value: dict[str, Any]) -> None:
except (TypeError, json.JSONDecodeError) as ex:
raise ValidationError("Invalid JSON configuration") from ex
- # Strict validation for all contexts - ensures consistent data quality
- if not is_valid_theme(theme_config):
- # Add detailed error info for debugging
- logger.error("Theme validation failed. Theme config: %s",
theme_config)
- logger.error(
- "Theme type: %s, Algorithm: %s, Has token: %s",
- type(theme_config).__name__,
- theme_config.get("algorithm"),
- "token" in theme_config,
- )
+ # Sanitize theme tokens (including SVG content)
+ sanitized_config = sanitize_theme_tokens(theme_config)
+
+ # Validate theme structure
+ if not is_valid_theme(sanitized_config):
raise ValidationError("Invalid theme configuration structure")
+ # Update the field with sanitized content for import
+ if sanitized_config != theme_config:
+ # Update the original value with sanitized content
+ if isinstance(value, dict):
+ value.clear()
+ value.update(sanitized_config)
+ else:
+ self.context["sanitized_json_data"] =
json.dumps(sanitized_config)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsafe mutation of validation input parameter</b></div>
<div id="fix">
The code attempts to mutate the input `value` parameter by calling
`value.clear()` and `value.update()`, but this will fail when `value` is not a
mutable dictionary. Marshmallow field values may be immutable or different
types, causing `AttributeError` during theme import validation. Store sanitized
data only in context instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
# Store sanitized content in context for import processing
self.context["sanitized_json_data"] =
json.dumps(sanitized_config)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/views/base.py:
##########
@@ -365,6 +378,31 @@ def get_theme_bootstrap_data() -> dict[str, Any]:
}
+def get_default_spinner_svg() -> str | None:
+ """
+ Load and cache the default spinner SVG content from frontend assets.
+
+ Returns:
+ str | None: SVG content as string, or None if file not found
+ """
+ try:
+ # Path to shared static assets SVG file
+ svg_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "static",
+ "assets",
+ "images",
+ "loading.svg",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect file path breaks spinner loading</b></div>
<div id="fix">
The file path construction points to a non-existent directory structure. The
function constructs path `superset/static/assets/images/loading.svg` but the
actual SVG file is located at
`superset-frontend/packages/superset-ui-core/src/components/assets/images/loading.svg`.
This will cause the function to always return `None` and log warnings,
breaking spinner functionality. Update the path to point to the correct
frontend assets location.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
svg_path = os.path.join(
os.path.dirname(__file__),
"..", "..",
"superset-frontend", "packages", "superset-ui-core",
"src", "components", "assets", "images",
"loading.svg",
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.test.tsx:
##########
@@ -101,8 +100,8 @@ describe('DatasetPanel', () => {
},
);
- const blankDatasetImg = screen.getByAltText(ALT_LOADING);
- expect(blankDatasetImg).toBeVisible();
+ const loadingIndicator = screen.getByTestId('loading-indicator');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test selector attribute mismatch</b></div>
<div id="fix">
The test is using `getByTestId('loading-indicator')` but the Loading
component uses `data-test="loading-indicator"` instead of
`data-testid="loading-indicator"`. This will cause the test to fail. Use
`getByRole('status')` instead since the Loading component has `role="status"`
which is the correct semantic approach.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
const loadingIndicator = screen.getByRole('status');
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/themes/utils.py:
##########
@@ -87,3 +88,35 @@ def is_valid_theme(theme: Dict[str, Any]) -> bool:
return True
except Exception:
return False
+
+
+def sanitize_theme_tokens(theme_config: Dict[str, Any]) -> Dict[str, Any]:
+ """Sanitize theme configuration, focusing on potentially dangerous content.
+
+ Sanitizes both brandSpinnerSvg content and brandSpinnerUrl values to
prevent XSS.
+
+ Args:
+ theme_config: Theme configuration dictionary
+
+ Returns:
+ Dict[str, Any]: Sanitized theme configuration
+ """
+ if not isinstance(theme_config, dict):
+ return theme_config
+
+ # Create a copy to avoid modifying the original
+ sanitized_config = theme_config.copy()
+
+ # Sanitize SVG content and URLs in tokens
+ if "token" in sanitized_config and isinstance(sanitized_config["token"],
dict):
+ tokens = sanitized_config["token"].copy()
+
+ if "brandSpinnerSvg" in tokens and tokens["brandSpinnerSvg"]:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing type validation for SVG content</b></div>
<div id="fix">
The code passes `tokens["brandSpinnerSvg"]` to `sanitize_svg_content()`
without verifying it's a string. The `sanitize_svg_content()` function expects
a string parameter, but the token value could be any type (int, list, dict,
etc.), which would cause a runtime error. Add type checking: `if
"brandSpinnerSvg" in tokens and isinstance(tokens["brandSpinnerSvg"], str) and
tokens["brandSpinnerSvg"]:`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
if "brandSpinnerSvg" in tokens and
isinstance(tokens["brandSpinnerSvg"], str) and tokens["brandSpinnerSvg"]:
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/themes/utils.py:
##########
@@ -87,3 +88,35 @@
return True
except Exception:
return False
+
+
+def sanitize_theme_tokens(theme_config: Dict[str, Any]) -> Dict[str, Any]:
+ """Sanitize theme configuration, focusing on potentially dangerous content.
+
+ Sanitizes both brandSpinnerSvg content and brandSpinnerUrl values to
prevent XSS.
+
+ Args:
+ theme_config: Theme configuration dictionary
+
+ Returns:
+ Dict[str, Any]: Sanitized theme configuration
+ """
+ if not isinstance(theme_config, dict):
+ return theme_config
+
+ # Create a copy to avoid modifying the original
+ sanitized_config = theme_config.copy()
+
+ # Sanitize SVG content and URLs in tokens
+ if "token" in sanitized_config and isinstance(sanitized_config["token"],
dict):
+ tokens = sanitized_config["token"].copy()
+
+ if "brandSpinnerSvg" in tokens and tokens["brandSpinnerSvg"]:
+ tokens["brandSpinnerSvg"] =
sanitize_svg_content(tokens["brandSpinnerSvg"])
+
+ if "brandSpinnerUrl" in tokens and tokens["brandSpinnerUrl"]:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing type validation for URL content</b></div>
<div id="fix">
The code passes `tokens["brandSpinnerUrl"]` to `sanitize_url()` without
verifying it's a string. The `sanitize_url()` function expects a string
parameter, but the token value could be any type (int, list, dict, etc.), which
would cause a runtime error. Add type checking: `if "brandSpinnerUrl" in tokens
and isinstance(tokens["brandSpinnerUrl"], str) and tokens["brandSpinnerUrl"]:`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
if "brandSpinnerUrl" in tokens and
isinstance(tokens["brandSpinnerUrl"], str) and tokens["brandSpinnerUrl"]:
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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-frontend/packages/superset-ui-core/src/components/Loading/index.tsx:
##########
@@ -50,17 +62,47 @@ export function Loading({
position = 'floating',
image,
className,
+ size = 'm',
+ muted = false,
}: LoadingProps) {
+ const theme = useTheme();
+
+ // Determine size from size prop
+ const spinnerSize = SIZE_MAP[size];
+
+ // Opacity - muted reduces to 0.25, otherwise full opacity
+ const opacity = muted ? 0.25 : 1.0;
+
+ // Render spinner content
+ const renderSpinner = () => {
+ // Precedence: explicit image prop > brandSpinnerSvg > brandSpinnerUrl >
default SVG
+ if (image) {
+ return <img src={image} alt="Loading..." />;
+ }
+ if (theme.brandSpinnerSvg) {
+ const svgDataUri =
`data:image/svg+xml;base64,${btoa(theme.brandSpinnerSvg)}`;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>btoa function unavailable in Node.js</b></div>
<div id="fix">
The `btoa()` function is a browser-only Web API that will cause a
ReferenceError in Node.js environments during SSR or testing. Add a runtime
check: `const svgDataUri = \`data:image/svg+xml;base64,${typeof btoa !==
'undefined' ? btoa(theme.brandSpinnerSvg) :
Buffer.from(theme.brandSpinnerSvg).toString('base64')}\`;`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
const svgDataUri = `data:image/svg+xml;base64,${typeof btoa !==
'undefined' ? btoa(theme.brandSpinnerSvg) :
Buffer.from(theme.brandSpinnerSvg).toString('base64')}`;
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/views/core.py:
##########
@@ -824,17 +821,13 @@ def dashboard(
),
)
- return self.render_template(
- "superset/spa.html",
- entry="spa",
+ bootstrap_payload = {
+ "user": bootstrap_user_data(g.user, include_perms=True),
+ "common": common_bootstrap_payload(),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Data duplication causing value override</b></div>
<div id="fix">
Data duplication issue: `bootstrap_data` contains 'user' and 'common' keys
that will be overridden by `render_app_template`. The `get_spa_payload`
function automatically adds these keys, causing the manually set values to be
lost. Remove 'user' and 'common' from `bootstrap_data` to prevent data loss.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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-frontend/packages/superset-ui-core/src/components/Loading/index.test.tsx:
##########
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { render, screen } from '@superset-ui/core/spec';
+import * as themeModule from '../../theme';
+import { Loading } from '.';
+
+// Mock the loading SVG import since it's a file stub in tests
+jest.mock('../assets', () => ({
+ Loading: () => <svg data-test="default-loading-svg" />,
+}));
+
+const mockUseTheme = jest.fn();
+
+beforeEach(() => {
+ mockUseTheme.mockReset();
+ jest.spyOn(themeModule, 'useTheme').mockImplementation(mockUseTheme);
+});
+
+afterEach(() => {
+ jest.restoreAllMocks();
+});
+
+test('uses default spinner when no theme spinner configured', () => {
+ mockUseTheme.mockReturnValue({});
+
+ render(<Loading />);
+ const loading = screen.getByRole('status');
+ expect(loading).toBeInTheDocument();
+ // Now renders SVG component instead of img with src
+ expect(
+ loading.querySelector('[data-test="default-loading-svg"]'),
+ ).toBeInTheDocument();
+});
+
+test('uses brandSpinnerUrl from theme when configured', () => {
+ mockUseTheme.mockReturnValue({
+ brandSpinnerUrl: '/custom/spinner.png',
+ });
+
+ render(<Loading />);
+ const loading = screen.getByRole('status');
+ expect(loading).toBeInTheDocument();
+ const img = loading.querySelector('img');
+ expect(img).toHaveAttribute('src', '/custom/spinner.png');
+});
+
+test('uses brandSpinnerSvg from theme when configured', () => {
+ const svgContent = '<svg><circle cx="25" cy="25" r="20"/></svg>';
+ mockUseTheme.mockReturnValue({
+ brandSpinnerSvg: svgContent,
+ });
+
+ render(<Loading />);
+ const loading = screen.getByRole('status');
+ expect(loading).toBeInTheDocument();
+ const img = loading.querySelector('img');
+ const src = img?.getAttribute('src');
+ expect(src).toContain('data:image/svg+xml;base64,');
+ expect(atob(src!.split(',')[1])).toBe(svgContent);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsafe non-null assertion operator usage</b></div>
<div id="fix">
The test uses the non-null assertion operator (`!`) on `src` without proper
null checking. This could cause runtime errors if `src` is null or undefined.
Add proper null checking before using the non-null assertion operator.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
expect(src).toBeTruthy();
expect(atob(src!.split(',')[1])).toBe(svgContent);
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/views/core.py:
##########
@@ -824,17 +821,13 @@
),
)
- return self.render_template(
- "superset/spa.html",
- entry="spa",
+ bootstrap_payload = {
+ "user": bootstrap_user_data(g.user, include_perms=True),
+ "common": common_bootstrap_payload(),
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate bootstrap data creation causing
overwrite</b></div>
<div id="fix">
The `bootstrap_payload` manually creates 'user' and 'common' data, but
`render_app_template` internally calls `get_spa_payload` which also creates the
same data. This causes duplication and the manually created data will be
overwritten. Remove the manual creation and pass `None` or an empty dict for
`extra_bootstrap_data`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
bootstrap_payload = {}
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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/views/core.py:
##########
@@ -937,13 +930,7 @@
"common": common_bootstrap_payload(),
}
- return self.render_template(
- "superset/spa.html",
- entry="spa",
- bootstrap_data=json.dumps(
- payload, default=json.pessimistic_json_iso_dttm_ser
- ),
- )
+ return self.render_app_template(extra_bootstrap_data=payload)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Bootstrap data duplication in welcome method</b></div>
<div id="fix">
Data duplication issue: The `payload` containing `user` and `common` data is
passed as `extra_bootstrap_data`, but `render_app_template` internally calls
`get_spa_payload()` which already creates the same `user` and `common`
structure. This results in duplicate bootstrap data in the final payload.
Remove the manual payload creation and call `render_app_template()` without
parameters.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- payload = {
- "user": bootstrap_user_data(g.user, include_perms=True),
- "common": common_bootstrap_payload(),
- }
-
- return self.render_app_template(extra_bootstrap_data=payload)
+ return self.render_app_template()
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34764#issuecomment-3247402243>#e62369</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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
