codeant-ai-for-open-source[bot] commented on code in PR #36987:
URL: https://github.com/apache/superset/pull/36987#discussion_r2673858306
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -377,7 +392,21 @@ function AlertList({
original.owners.map((o: Owner) => o.id).includes(user.userId) ||
isUserAdmin(user);
+ const isRunning = original.lastState === AlertState.Working;
Review Comment:
**Suggestion:** Property-name mismatch: the cell code checks
`original.lastState` but the rest of the code and API use `last_state`
(snake_case). This causes the UI disabled state to be incorrect (the button may
be enabled/disabled incorrectly). Use the consistent `last_state` property on
`original`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const isRunning = original.last_state === AlertState.Working;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The UI checks original.lastState but the rest of the component (and API
data) uses snake_case last_state.
That mismatch will make isRunning always false and the Run Now button not
reflect the real running state.
Changing to original.last_state fixes a real logic bug and aligns with the
rest of the file where last_state is used.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 395:395
**Comment:**
*Logic Error: Property-name mismatch: the cell code checks
`original.lastState` but the rest of the code and API use `last_state`
(snake_case). This causes the UI disabled state to be incorrect (the button may
be enabled/disabled incorrectly). Use the consistent `last_state` property on
`original`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/reports/api.py:
##########
@@ -460,6 +465,38 @@ def put(self, pk: int) -> Response:
)
return self.response_422(message=str(ex))
+ @expose("/<int:pk>/run_now", methods=("POST",))
+ @protect()
Review Comment:
**Suggestion:** Missing permission check: the new `run_now` endpoint is
decorated with `@protect()` but has no `@permission_name(...)` decorator, so
any authenticated user may be allowed to trigger report execution even if they
shouldn't have permission; add the appropriate `@permission_name` decorator to
enforce the same permission model used by other endpoints (e.g. "post").
[security]
**Severity Level:** Critical 🚨
```suggestion
@permission_name("post")
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid security concern: other endpoints in this class use
@permission_name to enforce model-level permissions (e.g. "post", "put",
"delete"). @protect only enforces authentication; leaving out @permission_name
means an authenticated user could call run_now without the expected privilege.
Adding the proper @permission_name (likely "post" or a more specific
permission) matches repository patterns and fixes an authorization gap.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 469:469
**Comment:**
*Security: Missing permission check: the new `run_now` endpoint is
decorated with `@protect()` but has no `@permission_name(...)` decorator, so
any authenticated user may be allowed to trigger report execution even if they
shouldn't have permission; add the appropriate `@permission_name` decorator to
enforce the same permission model used by other endpoints (e.g. "post").
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/pages/AlertReportList/index.tsx:
##########
@@ -423,7 +452,7 @@ function AlertList({
id: QueryObjectColumns.ChangedBy,
},
],
- [canDelete, canEdit, isReportEnabled, toggleActive],
+ [canDelete, canEdit, isReportEnabled, toggleActive, refreshData],
Review Comment:
**Suggestion:** Missing dependencies in the `useMemo` columns dependency
array: `columns` uses `runReportNow` and `user` (closed over inside the column
cell), but they are not listed in the memo dependencies which can lead to stale
closures and UI not reflecting updated values; add them to the dependency
array. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
[canDelete, canEdit, isReportEnabled, toggleActive, refreshData,
runReportNow, user],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The columns use runReportNow and reference user inside cell renderers. Those
values are closed over by useMemo.
If runReportNow or user change and the memo doesn't re-run, the column cell
will keep stale references.
Adding them to the dependency array prevents subtle UI bugs. Ideally
runReportNow should be stabilized with useCallback to avoid unnecessary
recomputations.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/AlertReportList/index.tsx
**Line:** 455:455
**Comment:**
*Possible Bug: Missing dependencies in the `useMemo` columns dependency
array: `columns` uses `runReportNow` and `user` (closed over inside the column
cell), but they are not listed in the memo dependencies which can lead to stale
closures and UI not reflecting updated values; add them to the dependency array.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
