korbit-ai[bot] commented on code in PR #33675:
URL: https://github.com/apache/superset/pull/33675#discussion_r2124497892
##########
superset/css_templates/api.py:
##########
@@ -103,6 +105,13 @@ class CssTemplateRestApi(BaseSupersetModelRestApi):
"changed_by": [["id", BaseFilterRelatedUsers, lambda: []]],
}
+ @before_request()
+ def ensure_css_templates_enabled(self) -> Optional[Response]:
+ css_templates_enabled = is_feature_enabled("ENABLE_CSS_TEMPLATES")
+ if not css_templates_enabled:
+ return self.response_404("CSS templates are not enabled.")
+ return None
Review Comment:
### Redundant Feature Flag Checks <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The feature flag check is performed on every API request, causing
unnecessary repeated calls to `is_feature_enabled()`
###### Why this matters
Since feature flags typically don't change during runtime, checking the flag
on every request adds unnecessary overhead, especially under high traffic
conditions
###### Suggested change ∙ *Feature Preview*
Cache the feature flag value at class initialization or use a class-level
property to store the result. Example:
```python
class CssTemplateRestApi(BaseSupersetModelRestApi):
def __init__(self):
super().__init__()
self._css_templates_enabled =
is_feature_enabled("ENABLE_CSS_TEMPLATES")
@before_request()
def ensure_css_templates_enabled(self) -> Optional[Response]:
if not self._css_templates_enabled:
return self.response_404("CSS templates are not enabled.")
return None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fadab07-d92c-463d-b7df-23d5c1968b56/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fadab07-d92c-463d-b7df-23d5c1968b56?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fadab07-d92c-463d-b7df-23d5c1968b56?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fadab07-d92c-463d-b7df-23d5c1968b56?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7fadab07-d92c-463d-b7df-23d5c1968b56)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:539e27b0-eefc-4605-a171-aeb7b166b651 -->
[](539e27b0-eefc-4605-a171-aeb7b166b651)
##########
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -192,7 +192,7 @@ export const useHeaderActionsMenu = ({
{t('Edit properties')}
</Menu.Item>
)}
- {editMode && (
+ {editMode && isFeatureEnabled(FeatureFlag.EnableCssTemplates) && (
Review Comment:
### Missing Permission Check for CSS Editor <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The CSS Editor menu item visibility check combines editMode with feature
flag check but doesn't validate if userCanEdit is true.
###### Why this matters
A user without edit permissions might still see the CSS Editor menu item if
they somehow enter edit mode, potentially leading to unauthorized access
attempts or confusing UI state.
###### Suggested change ∙ *Feature Preview*
Add userCanEdit to the conditional check for showing the CSS Editor menu
item:
```typescript
{editMode && userCanEdit && isFeatureEnabled(FeatureFlag.EnableCssTemplates)
&& (
<Menu.Item key={MenuKeys.EditCss}>
<CssEditor
triggerNode={<div>{t('Edit CSS')}</div>}
initialCss={css}
onChange={changeCss}
addDangerToast={addDangerToast}
/>
</Menu.Item>
)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7ad2eab-45e1-4d5f-bd16-010d6c5b18f9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7ad2eab-45e1-4d5f-bd16-010d6c5b18f9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7ad2eab-45e1-4d5f-bd16-010d6c5b18f9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7ad2eab-45e1-4d5f-bd16-010d6c5b18f9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7ad2eab-45e1-4d5f-bd16-010d6c5b18f9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fc11eef7-7dfc-454d-a1a5-27a331650cea -->
[](fc11eef7-7dfc-454d-a1a5-27a331650cea)
##########
superset/css_templates/api.py:
##########
@@ -103,6 +105,13 @@
"changed_by": [["id", BaseFilterRelatedUsers, lambda: []]],
}
+ @before_request()
+ def ensure_css_templates_enabled(self) -> Optional[Response]:
+ css_templates_enabled = is_feature_enabled("ENABLE_CSS_TEMPLATES")
+ if not css_templates_enabled:
+ return self.response_404("CSS templates are not enabled.")
Review Comment:
### Incorrect HTTP status code for disabled feature <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using HTTP 404 (Not Found) status code when CSS templates are disabled is
semantically incorrect. This should return 403 (Forbidden) instead since the
resource exists but access is not allowed.
###### Why this matters
Using 404 could mislead clients and monitoring systems to believe the
endpoint doesn't exist, when in fact it does exist but is deliberately
disabled. This can complicate debugging and create confusion about whether
there's a routing issue versus a permissions issue.
###### Suggested change ∙ *Feature Preview*
Replace the response_404 with response_403:
```python
@before_request()
def ensure_css_templates_enabled(self) -> Optional[Response]:
css_templates_enabled = is_feature_enabled("ENABLE_CSS_TEMPLATES")
if not css_templates_enabled:
return self.response_403("CSS templates are not enabled.")
return None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b9d1133-4dc6-4cc2-82f1-b2ed1678fe9e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b9d1133-4dc6-4cc2-82f1-b2ed1678fe9e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b9d1133-4dc6-4cc2-82f1-b2ed1678fe9e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b9d1133-4dc6-4cc2-82f1-b2ed1678fe9e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b9d1133-4dc6-4cc2-82f1-b2ed1678fe9e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:787b04e7-d6cc-4b3d-990e-609416c77039 -->
[](787b04e7-d6cc-4b3d-990e-609416c77039)
##########
superset/config.py:
##########
@@ -1155,8 +1157,8 @@ class CeleryConfig: # pylint:
disable=too-few-public-methods
# else:
# return f'tmp_{schema}'
# Function accepts database object, user object, schema name and sql that will
be run.
-SQLLAB_CTAS_SCHEMA_NAME_FUNC: (
- None | (Callable[[Database, models.User, str, str], str])
+SQLLAB_CTAS_SCHEMA_NAME_FUNC: None | (
+ Callable[[Database, models.User, str, str], str]
) = None
Review Comment:
### Hard to read type annotation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The complex type annotation for the callable is hard to read due to nested
parentheses and line wrapping.
###### Why this matters
Complex type hints that are hard to parse visually make it difficult to
understand the expected function signature quickly.
###### Suggested change ∙ *Feature Preview*
```python
# Define a type alias for better readability
SchemaNameCallable = Callable[[Database, models.User, str, str], str]
SQLLAB_CTAS_SCHEMA_NAME_FUNC: SchemaNameCallable | None = None
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/64cd6e2c-4523-4d3d-9a43-a19c29ba44dc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/64cd6e2c-4523-4d3d-9a43-a19c29ba44dc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/64cd6e2c-4523-4d3d-9a43-a19c29ba44dc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/64cd6e2c-4523-4d3d-9a43-a19c29ba44dc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/64cd6e2c-4523-4d3d-9a43-a19c29ba44dc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:43557cef-a73f-4897-9987-b8078a79b434 -->
[](43557cef-a73f-4897-9987-b8078a79b434)
--
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
