korbit-ai[bot] commented on code in PR #35083:
URL: https://github.com/apache/superset/pull/35083#discussion_r2335902160
##########
superset-frontend/src/components/Modal/ModalFormField.tsx:
##########
@@ -48,7 +51,7 @@ const StyledFieldContainer = styled.div<{ bottomSpacing:
boolean }>`
.required {
margin-left: ${theme.sizeUnit / 2}px;
- color: ${theme.colorError};
+ color: ${hasError ? theme.colorError : theme.colorIcon};
Review Comment:
### Inconsistent Required Field Indicator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The asterisk (*) indicating a required field changes color from error to
icon color when there's no error, which conflicts with form validation best
practices.
###### Why this matters
Inconsistent visual representation of required fields can confuse users and
make it harder to identify mandatory fields at a glance, especially when
switching between forms with and without validation errors.
###### Suggested change ∙ *Feature Preview*
Keep the required field asterisk consistently red by removing the
conditional color change:
```typescript
color: ${theme.colorError};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef46fe38-88f2-4cfe-b0c8-fbfda8035169/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef46fe38-88f2-4cfe-b0c8-fbfda8035169?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef46fe38-88f2-4cfe-b0c8-fbfda8035169?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef46fe38-88f2-4cfe-b0c8-fbfda8035169?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef46fe38-88f2-4cfe-b0c8-fbfda8035169)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a2b852ae-2155-4435-8483-79dbce34fad9 -->
[](a2b852ae-2155-4435-8483-79dbce34fad9)
##########
superset/commands/report/execute_now.py:
##########
@@ -0,0 +1,147 @@
+# 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 logging
+from typing import Optional
+from uuid import uuid4
+
+from flask import current_app
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.exceptions import CommandException
+from superset.commands.report.exceptions import (
+ ReportScheduleCeleryNotConfiguredError,
+ ReportScheduleExecuteNowFailedError,
+ ReportScheduleForbiddenError,
+ ReportScheduleNotFoundError,
+)
+from superset.daos.report import ReportScheduleDAO
+from superset.exceptions import SupersetSecurityException
+from superset.reports.models import ReportSchedule
+from superset.utils.decorators import transaction
+
+logger = logging.getLogger(__name__)
+
+
+class ExecuteReportScheduleNowCommand(BaseCommand):
+ """
+ Execute a report schedule immediately (manual trigger).
+
+ This command validates permissions and triggers immediate execution
+ of a report or alert via Celery task, similar to scheduled execution
+ but without waiting for the cron schedule.
+ """
+
+ def __init__(self, model_id: int) -> None:
+ self._model_id = model_id
+ self._model: Optional[ReportSchedule] = None
+
+ @transaction()
+ def run(self) -> str:
Review Comment:
### Overly Broad Transaction Scope <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The entire report execution process is wrapped in a database transaction,
which could hold database connections longer than necessary.
###### Why this matters
Long-running transactions can lead to database connection pool exhaustion
and reduce system scalability, especially since the actual report execution is
asynchronous.
###### Suggested change ∙ *Feature Preview*
Move the transaction decorator to only wrap the necessary database
operations, likely just the validation step:
```python
def run(self) -> str:
self.validate()
# Rest of the execution logic
@transaction()
def validate(self) -> None:
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc1b03b8-0fd6-4007-952b-87913217d04f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc1b03b8-0fd6-4007-952b-87913217d04f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc1b03b8-0fd6-4007-952b-87913217d04f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc1b03b8-0fd6-4007-952b-87913217d04f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc1b03b8-0fd6-4007-952b-87913217d04f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ad6ae18f-08b2-44ca-9fdc-cc2f4f9af69f -->
[](ad6ae18f-08b2-44ca-9fdc-cc2f4f9af69f)
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -246,6 +251,51 @@ function AlertList({
[alerts, setResourceCollection, updateResource],
);
+ const handleExecuteReport = useCallback(
+ async (alert: AlertObject) => {
+ const alertId = alert.id;
+ if (!alertId || executingIds.has(alertId)) {
+ return;
+ }
+
+ // Add to executing set
+ setExecutingIds(prev => new Set(prev).add(alertId));
Review Comment:
### Inefficient Set Updates <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new Set and copying all elements on every execution state update
is inefficient.
###### Why this matters
Unnecessary creation of intermediate Set objects and copying of all elements
impacts performance when handling multiple executions.
###### Suggested change ∙ *Feature Preview*
Use the spread operator with Array conversion for more efficient updates:
```typescript
setExecutingIds(prev => new Set([...prev, alertId]));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a77f8296-e09b-4f79-87c6-b304ce2e2805/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a77f8296-e09b-4f79-87c6-b304ce2e2805?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a77f8296-e09b-4f79-87c6-b304ce2e2805?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a77f8296-e09b-4f79-87c6-b304ce2e2805?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a77f8296-e09b-4f79-87c6-b304ce2e2805)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f97790f4-77c5-4e03-8991-c2f08421680f -->
[](f97790f4-77c5-4e03-8991-c2f08421680f)
##########
superset/commands/report/execute_now.py:
##########
@@ -0,0 +1,147 @@
+# 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 logging
+from typing import Optional
+from uuid import uuid4
+
+from flask import current_app
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.exceptions import CommandException
+from superset.commands.report.exceptions import (
+ ReportScheduleCeleryNotConfiguredError,
+ ReportScheduleExecuteNowFailedError,
+ ReportScheduleForbiddenError,
+ ReportScheduleNotFoundError,
+)
+from superset.daos.report import ReportScheduleDAO
+from superset.exceptions import SupersetSecurityException
+from superset.reports.models import ReportSchedule
+from superset.utils.decorators import transaction
+
+logger = logging.getLogger(__name__)
+
+
+class ExecuteReportScheduleNowCommand(BaseCommand):
+ """
+ Execute a report schedule immediately (manual trigger).
+
+ This command validates permissions and triggers immediate execution
+ of a report or alert via Celery task, similar to scheduled execution
+ but without waiting for the cron schedule.
+ """
+
+ def __init__(self, model_id: int) -> None:
+ self._model_id = model_id
+ self._model: Optional[ReportSchedule] = None
+
+ @transaction()
+ def run(self) -> str:
+ """
+ Execute the command and return execution UUID for tracking.
+
+ Returns:
+ str: Execution UUID that can be used to track the execution status
+
+ Raises:
+ ReportScheduleNotFoundError: Report schedule not found
+ ReportScheduleForbiddenError: User doesn't have permission to
execute
+ ReportScheduleExecuteNowFailedError: Execution failed to start
+ """
+ try:
+ self.validate()
+ if not self._model:
+ raise ReportScheduleExecuteNowFailedError()
+
+ # Generate execution UUID for tracking
+ execution_id = str(uuid4())
+
+ # Trigger immediate execution via Celery
+ logger.info(
+ "Manually executing report schedule %s (id: %d), execution_id:
%s",
+ self._model.name,
+ self._model.id,
+ execution_id,
+ )
+
+ # Import the existing execute task to avoid circular imports
+ from superset.tasks.scheduler import execute
+
+ # Set async options similar to scheduler but for immediate
execution
+ async_options = {"task_id": execution_id}
+ if self._model.working_timeout is not None and
current_app.config.get(
+ "ALERT_REPORTS_WORKING_TIME_OUT_KILL", True
+ ):
+ async_options["time_limit"] = (
+ self._model.working_timeout
+ +
current_app.config.get("ALERT_REPORTS_WORKING_TIME_OUT_LAG", 10)
+ )
+ async_options["soft_time_limit"] = (
+ self._model.working_timeout
+ + current_app.config.get(
+ "ALERT_REPORTS_WORKING_SOFT_TIME_OUT_LAG", 5
+ )
+ )
+
+ # Execute the task
+ try:
+ execute.apply_async((self._model.id,), **async_options)
+ except Exception as celery_ex:
+ # Check for common Celery configuration issues
+ error_msg = str(celery_ex).lower()
+ if any(
+ keyword in error_msg
+ for keyword in [
+ "no broker",
+ "broker connection",
+ "kombu",
+ "redis",
+ "rabbitmq",
+ "celery",
+ "not registered",
+ "connection refused",
+ ]
+ ):
Review Comment:
### Inefficient Error Type Checking <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using string matching in a loop to check error types is inefficient and
potentially unreliable. Each iteration performs a substring search operation.
###### Why this matters
String matching in a loop can be computationally expensive, especially with
multiple keywords. For error handling, it's more efficient to catch specific
exception types.
###### Suggested change ∙ *Feature Preview*
Replace string matching with specific exception type checking. Use
try/except blocks with specific Celery exceptions:
```python
try:
execute.apply_async((self._model.id,), **async_options)
except (kombu.exceptions.OperationalError,
celery.exceptions.NotRegistered,
redis.exceptions.ConnectionError) as ex:
logger.error("Celery backend not configured: %s", str(ex))
raise ReportScheduleCeleryNotConfiguredError() from ex
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/423a4d9f-95ee-47aa-ae25-ed372ad40801/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/423a4d9f-95ee-47aa-ae25-ed372ad40801?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/423a4d9f-95ee-47aa-ae25-ed372ad40801?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/423a4d9f-95ee-47aa-ae25-ed372ad40801?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/423a4d9f-95ee-47aa-ae25-ed372ad40801)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9c65c8e3-9184-4c04-a4a4-c9a9f3d2333e -->
[](9c65c8e3-9184-4c04-a4a4-c9a9f3d2333e)
--
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
