codeant-ai-for-open-source[bot] commented on code in PR #32995:
URL: https://github.com/apache/superset/pull/32995#discussion_r3593245849
##########
superset-frontend/src/pages/ChartList/index.tsx:
##########
@@ -517,7 +516,8 @@ function ChartList(props: ChartListProps) {
id: 'changed_on_delta_humanized',
},
{
- Cell: ({ row: { original } }: any) => {
+ Cell: ({ row: { original } }: CellProps<Chart>) => {
+ const allowEdit = isUserEditorOrAdmin(user, original.editors);
Review Comment:
**Suggestion:** `allowEdit` now depends on the full `user` object, but the
`columns` memoization is still keyed by the old dependency set (it does not
include `user`). If the current user context changes without a full remount
(for example role updates/impersonation flows), action buttons keep using stale
permission data and can remain incorrectly enabled/disabled. Include `user` in
the `useMemo` dependency list for `columns`. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Chart list actions show outdated edit/delete availability.
⚠️ Stale permissions after role updates without page reload.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Navigate to the charts list page, which renders `ChartList` from
`superset-frontend/src/pages/ChartList/index.tsx` via the route
configuration in
`superset-frontend/src/views/routes.tsx:49-53`.
2. Observe that the `columns` definition is memoized with `useMemo` at
`superset-frontend/src/pages/ChartList/index.tsx:79-113`, and its dependency
array (lines
101-113) includes `userId` but not the full `user` object or `user.roles`.
3. Inside that memoized `columns` array, the Actions column’s `Cell` closure
(lines
519-587) computes `const allowEdit = isUserEditorOrAdmin(user,
original.editors);` (line
520), where `isUserEditorOrAdmin` from
`superset-frontend/src/dashboard/util/permissionUtils.ts:49-52` checks
`isUserAdmin(user)`
and therefore depends on `user.roles`.
4. If `ChartList` is re-used in the same React tree with a new `user` prop
whose `userId`
is unchanged but whose `roles` differ (e.g., in a test, call
`renderChartList` in
`superset-frontend/src/pages/ChartList/ChartList.permissions.test.tsx:18-27`
with a new
`user` object but same `userId`), `useMemo` will not recompute `columns`
because only
`userId` is tracked; the Actions column `Cell` continues to capture the old
`user` and
thus old admin/editor status, so edit/delete buttons remain enabled/disabled
based on
stale permissions until a full remount or dependency change.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c0f6458e45041c0a2975c340607166a&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=8c0f6458e45041c0a2975c340607166a&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:** 520:520
**Comment:**
*Stale Reference: `allowEdit` now depends on the full `user` object,
but the `columns` memoization is still keyed by the old dependency set (it does
not include `user`). If the current user context changes without a full remount
(for example role updates/impersonation flows), action buttons keep using stale
permission data and can remain incorrectly enabled/disabled. Include `user` in
the `useMemo` dependency list for `columns`.
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=194bd0341cc461b249d9ccd838a684243874392887c7951da53e09c8269604cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=194bd0341cc461b249d9ccd838a684243874392887c7951da53e09c8269604cf&reaction=dislike'>👎</a>
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -479,7 +481,8 @@ function DashboardList(props: DashboardListProps) {
id: 'changed_on_delta_humanized',
},
{
- Cell: ({ row: { original } }: any) => {
+ Cell: ({ row: { original } }: CellProps<Dashboard>) => {
+ const allowEdit = isUserEditorOrAdmin(user, original.editors);
Review Comment:
**Suggestion:** This row action logic now reads `user` to compute
edit/delete availability, but the `columns` memoization still only tracks the
previous dependency set (not the full `user` object). If user roles/identity
context changes during app lifetime, the UI can show outdated edit/delete
enablement. Add `user` to the `useMemo` dependency array for `columns`. [stale
reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Dashboard list actions show outdated edit/delete availability.
⚠️ Stale dashboard permissions after mid-session role changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Navigate to the dashboards list page, which renders `DashboardList` from
`superset-frontend/src/pages/DashboardList/index.tsx` via the route
configuration in
`superset-frontend/src/views/routes.tsx:49-52`.
2. Note that `DashboardList` defines a memoized `columns` array with
`useMemo` at
`superset-frontend/src/pages/DashboardList/index.tsx:91-155`, whose
dependency array
(lines 68-80 in the same file) includes `user?.userId` but not the full
`user` object or
its `roles`.
3. The Actions column `Cell` (lines 86-144 in
`superset-frontend/src/pages/DashboardList/index.tsx`) computes `const
allowEdit =
isUserEditorOrAdmin(user, original.editors);` (line 86/485), and
`isUserEditorOrAdmin`
uses `isUserAdmin(user)` from
`superset-frontend/src/dashboard/util/permissionUtils.ts:41-52`, which
depends on
`user.roles`.
4. If `DashboardList` is rendered in a context where the `user` prop changes
(for example,
in `renderDashboardListWithPermissions` in
`superset-frontend/src/pages/DashboardList/DashboardList.permissions.test.tsx:36-47`,
first with an admin `user` and then with a non-admin `user` sharing the same
`userId`),
the `useMemo` dependency array does not see a change (since `user?.userId`
is unchanged),
so the memoized `columns` and their `Cell` closures keep capturing the old
`user`; as a
result, edit/delete IconButtons in the Actions column continue to be enabled
or disabled
based on stale admin/editor status until a full remount or dependency change.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e567e0fedeab49bb84b7dd30fa87153e&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=e567e0fedeab49bb84b7dd30fa87153e&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/DashboardList/index.tsx
**Line:** 485:485
**Comment:**
*Stale Reference: This row action logic now reads `user` to compute
edit/delete availability, but the `columns` memoization still only tracks the
previous dependency set (not the full `user` object). If user roles/identity
context changes during app lifetime, the UI can show outdated edit/delete
enablement. Add `user` to the `useMemo` dependency array for `columns`.
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=d5c8ef60b86dfeb0b4f23b0967a946762e210fd58e520ebd7df1adbda709eb6a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=d5c8ef60b86dfeb0b4f23b0967a946762e210fd58e520ebd7df1adbda709eb6a&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]
