korbit-ai[bot] commented on code in PR #34764:
URL: https://github.com/apache/superset/pull/34764#discussion_r2286188310
##########
superset/templates/superset/spa.html:
##########
@@ -70,16 +70,41 @@
{% block body %}
<div id="app" data-bootstrap="{{ bootstrap_data }}">
- <img
- src="{{ assets_prefix }}/static/assets/images/loading.gif"
- style="
- width: 50px;
- position: absolute;
- top: 50%;
- left: 50%;
- transform: translate(-50%, -50%);
- "
- />
+ {% set default_theme = theme_data.get('default', {}) %}
+ {% set theme_tokens = default_theme.get('token', {}) %}
+
+ {% if theme_tokens.get('brandSpinnerSvg') %}
+ <!-- SVG Priority: Inline SVG content -->
+ <div style="width: 50px; position: absolute; top: 50%; left: 50%;
transform: translate(-50%, -50%);">
Review Comment:
### Redundant Inline Styles <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Duplicate inline styles repeated three times in the template causing
unnecessary code bloat.
###### Why this matters
Redundant CSS definitions increase the HTML size and parsing time
unnecessarily.
###### Suggested change ∙ *Feature Preview*
Extract the common styles into a CSS class in the stylesheet:
```html
<style>
.centered-spinner {
width: 50px;
position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
}
</style>
```
Then use `class="centered-spinner"` in the markup.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/15bd0529-77b3-41b2-a0db-948b4bec5975/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/15bd0529-77b3-41b2-a0db-948b4bec5975?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/15bd0529-77b3-41b2-a0db-948b4bec5975?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/15bd0529-77b3-41b2-a0db-948b4bec5975?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/15bd0529-77b3-41b2-a0db-948b4bec5975)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:abd1ceb4-0398-43b7-b68a-0f6caf8849c8 -->
[](abd1ceb4-0398-43b7-b68a-0f6caf8849c8)
##########
superset/templates/superset/spa.html:
##########
@@ -70,16 +70,41 @@
{% block body %}
<div id="app" data-bootstrap="{{ bootstrap_data }}">
- <img
- src="{{ assets_prefix }}/static/assets/images/loading.gif"
- style="
- width: 50px;
- position: absolute;
- top: 50%;
- left: 50%;
- transform: translate(-50%, -50%);
- "
- />
+ {% set default_theme = theme_data.get('default', {}) %}
+ {% set theme_tokens = default_theme.get('token', {}) %}
+
+ {% if theme_tokens.get('brandSpinnerSvg') %}
+ <!-- SVG Priority: Inline SVG content -->
+ <div style="width: 50px; position: absolute; top: 50%; left: 50%;
transform: translate(-50%, -50%);">
+ {{ theme_tokens.brandSpinnerSvg | safe }}
Review Comment:
### Unsafe SVG Rendering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unescaped SVG content is being rendered directly into the HTML using the
'safe' filter, which could lead to XSS vulnerabilities if the
theme_tokens.brandSpinnerSvg content is not properly validated.
###### Why this matters
If the SVG content contains malicious JavaScript code and isn't properly
sanitized before being stored in theme_tokens, it could execute in users'
browsers when the page loads.
###### Suggested change ∙ *Feature Preview*
Implement SVG content validation and sanitization before storing it in
theme_tokens. Consider using a dedicated SVG sanitizer library or strict
validation rules to ensure only safe SVG content is allowed.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ab4137e-627b-4140-81fb-4df744a41d76/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ab4137e-627b-4140-81fb-4df744a41d76?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ab4137e-627b-4140-81fb-4df744a41d76?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ab4137e-627b-4140-81fb-4df744a41d76?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ab4137e-627b-4140-81fb-4df744a41d76)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4600fe37-8423-4b03-a6b8-f01552054e9f -->
[](4600fe37-8423-4b03-a6b8-f01552054e9f)
##########
superset-frontend/packages/superset-ui-core/src/components/Loading/index.tsx:
##########
@@ -51,11 +51,23 @@
image,
className,
}: LoadingProps) {
+ const theme = useTheme();
+
+ // Precedence: explicit image prop > brandSpinnerSvg > brandSpinnerUrl >
default
+ const getSpinnerSource = () => {
+ if (image) return image; // Explicit prop takes highest precedence
+ if (theme.brandSpinnerSvg) {
+ return `data:image/svg+xml;base64,${btoa(theme.brandSpinnerSvg)}`;
+ }
+ if (theme.brandSpinnerUrl) return theme.brandSpinnerUrl;
+ return Loader; // Default loading.gif
+ };
Review Comment:
### Move Pure Function Outside Component <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getSpinnerSource function is defined inside the component, causing it to
be recreated on every render. This function doesn't depend on any props or
state that change frequently.
###### Why this matters
Recreating this function on every render is unnecessary and impacts
performance optimization techniques like memoization. It also makes the
function less reusable across components.
###### Suggested change ∙ *Feature Preview*
```typescript
const getSpinnerSource = (image: string | undefined, theme: Theme) => {
if (image) return image;
if (theme.brandSpinnerSvg) {
return `data:image/svg+xml;base64,${btoa(theme.brandSpinnerSvg)}`;
}
if (theme.brandSpinnerUrl) return theme.brandSpinnerUrl;
return Loader;
};
export function Loading({ position = 'floating', image, className }:
LoadingProps) {
const theme = useTheme();
return (
<LoaderImg
...
src={getSpinnerSource(image, theme)}
...
/>
);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2fdd124-151e-4ed8-b049-8a9c6364b49f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2fdd124-151e-4ed8-b049-8a9c6364b49f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2fdd124-151e-4ed8-b049-8a9c6364b49f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2fdd124-151e-4ed8-b049-8a9c6364b49f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2fdd124-151e-4ed8-b049-8a9c6364b49f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:612b1e1d-6eea-4f89-a99c-0ac277c9df51 -->
[](612b1e1d-6eea-4f89-a99c-0ac277c9df51)
##########
superset/initialization/__init__.py:
##########
@@ -630,15 +630,24 @@ def apply_http_headers(response: Response) -> Response:
def get_common_bootstrap_data() -> dict[str, Any]:
# Import here to avoid circular imports
from superset.utils import json
- from superset.views.base import common_bootstrap_payload
+ from superset.views.base import (
+ common_bootstrap_payload,
+ get_theme_bootstrap_data,
+ )
def serialize_bootstrap_data() -> str:
return json.dumps(
{"common": common_bootstrap_payload()},
default=json.pessimistic_json_iso_dttm_ser,
)
- return {"bootstrap_data": serialize_bootstrap_data}
+ # Get theme data for direct template access
+ theme_data = get_theme_bootstrap_data()
+
+ return {
+ "bootstrap_data": serialize_bootstrap_data,
+ "theme_data": theme_data.get("theme", {}),
+ }
Review Comment:
### Missing Theme Data Null Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The theme_data dictionary is accessed without checking if
get_theme_bootstrap_data() returned None, which could lead to a NoneType error
if the theme data retrieval fails.
###### Why this matters
If theme data retrieval fails, the application could crash when trying to
call .get() on None, preventing the page from loading.
###### Suggested change ∙ *Feature Preview*
Add a null check before accessing the theme data:
```python
theme_data = get_theme_bootstrap_data() or {}
return {
"bootstrap_data": serialize_bootstrap_data,
"theme_data": theme_data.get("theme", {}),
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2dc0e7b-7357-41e9-95e0-f00d865b98f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2dc0e7b-7357-41e9-95e0-f00d865b98f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2dc0e7b-7357-41e9-95e0-f00d865b98f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2dc0e7b-7357-41e9-95e0-f00d865b98f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2dc0e7b-7357-41e9-95e0-f00d865b98f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6cb589fc-0670-489c-b35d-f2f83426c0fd -->
[](6cb589fc-0670-489c-b35d-f2f83426c0fd)
##########
superset-frontend/packages/superset-ui-core/src/components/Loading/index.tsx:
##########
@@ -51,11 +51,23 @@ export function Loading({
image,
className,
}: LoadingProps) {
+ const theme = useTheme();
+
+ // Precedence: explicit image prop > brandSpinnerSvg > brandSpinnerUrl >
default
+ const getSpinnerSource = () => {
+ if (image) return image; // Explicit prop takes highest precedence
+ if (theme.brandSpinnerSvg) {
+ return `data:image/svg+xml;base64,${btoa(theme.brandSpinnerSvg)}`;
Review Comment:
### Unsafe SVG Base64 Encoding <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using btoa() for SVG base64 encoding can fail with Unicode characters in the
SVG content, as btoa() only works with ASCII characters.
###### Why this matters
If the SVG contains any non-ASCII characters (like in attributes or
content), the code will throw a 'Character Out Of Range' error, causing the
loading spinner to fail.
###### Suggested change ∙ *Feature Preview*
Use a safe base64 encoding function that handles Unicode characters
correctly:
```typescript
const toBase64 = (str: string) => {
try {
return btoa(unescape(encodeURIComponent(str)));
} catch (e) {
console.error('Failed to encode SVG:', e);
return '';
}
}
// Usage in getSpinnerSource
if (theme.brandSpinnerSvg) {
return `data:image/svg+xml;base64,${toBase64(theme.brandSpinnerSvg)}`;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/538697fb-eb9c-4213-ab6a-39e8423d99f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/538697fb-eb9c-4213-ab6a-39e8423d99f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/538697fb-eb9c-4213-ab6a-39e8423d99f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/538697fb-eb9c-4213-ab6a-39e8423d99f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/538697fb-eb9c-4213-ab6a-39e8423d99f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:50c4ad19-b0ea-4580-8e76-c3112409c7ae -->
[](50c4ad19-b0ea-4580-8e76-c3112409c7ae)
--
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]
