codeant-ai-for-open-source[bot] commented on code in PR #32995:
URL: https://github.com/apache/superset/pull/32995#discussion_r3524077124
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -501,6 +521,11 @@ function ChartList(props: ChartListProps) {
},
{
Cell: ({ row: { original } }: any) => {
+ // Verify owner or isAdmin
+ const allowEdit: boolean =
+ original.owners.map((o: Owner) => o.id).includes(user.userId) ||
+ isUserAdmin(user);
Review Comment:
**Suggestion:** The ownership check compares numeric owner IDs against
`user.userId` without normalizing types. Since `userId` is typed as `string |
number`, string IDs will fail `.includes(...)` even when the user is actually
an owner, incorrectly disabling edit/delete actions for valid owners. Normalize
`user.userId` to a number (as done in DatasetList) before the comparison. [type
error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Chart list page /chart/list blocks owner edits.
- β οΈ Owners see misleading tooltip about lacking edit permissions.
- β οΈ Inconsistent behavior versus dataset list owner checks.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Navigate to the Chart list page routed via `RoutePaths.CHART_LIST` in
`superset-frontend/src/views/routes.tsx:20-22,210-212`, which lazily loads
`src/pages/ChartList` as the component for the `/chart/list` path.
2. In `superset-frontend/src/pages/ChartList/index.tsx:160-168`, observe that
`ChartListProps` defines `user: { userId: string | number; ... }`, meaning
the `userId`
passed into `ChartList` can legitimately be either a string or a number at
runtime.
3. Create or identify a chart whose `owners` array contains an `Owner` with
a numeric `id`
matching the logged-in user (Owner model defined in
`superset-frontend/src/types/Owner.ts:24-27` with `id: number`), then ensure
the userβs
`userId` is set as a string containing the same digits (e.g. owners include
`{ id: 5 }`
while `user.userId === '5'`).
4. Load the chart list; in the Actions column renderer at
`superset-frontend/src/pages/ChartList/index.tsx:520-528`, `allowEdit` is
computed as
`original.owners.map((o: Owner) => o.id).includes(user.userId) ||
isUserAdmin(user)`.
Because the array contains numbers and `user.userId` is a string,
`.includes(user.userId)`
returns `false`, and if `isUserAdmin(user)` is also `false`, `allowEdit` is
`false`,
causing the Edit/Delete spans (lines 37-45 and 9-17 in the same block) to
get the
`disabled` class and no `onClick` handler. The logged-in owner therefore
cannot edit or
delete their own chart even though their numeric owner ID matches the string
`userId`,
while the analogous Dataset list implementation
(`superset-frontend/src/pages/DatasetList/index.tsx:880-917`) explicitly uses
`.includes(Number(user.userId))` to avoid this mismatch.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db09bab936be4030b2a706d729a52b0b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=db09bab936be4030b2a706d729a52b0b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ChartList/index.tsx
**Line:** 525:527
**Comment:**
*Type Error: The ownership check compares numeric owner IDs against
`user.userId` without normalizing types. Since `userId` is typed as `string |
number`, string IDs will fail `.includes(...)` even when the user is actually
an owner, incorrectly disabling edit/delete actions for valid owners. Normalize
`user.userId` to a number (as done in DatasetList) before the comparison.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=3d1f97a5fa84511cb534ffabd99deb78289a8f2a6396474d201b61dbe1baccb1&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=3d1f97a5fa84511cb534ffabd99deb78289a8f2a6396474d201b61dbe1baccb1&reaction=dislike'>π</a>
--
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]
