fix(roles): resolve HTTP 429 and 414 errors on role management page#39465
Draft
fix(roles): resolve HTTP 429 and 414 errors on role management page#39465
Conversation
- Replace permissions hydration in edit modal with GET
/api/v1/security/roles/{id}/permissions/ — one request per role,
role ID in path so URL length is constant regardless of permission count
(fixes 414 URI Too Long for roles with many permissions)
- Batch fetchPaginatedData page requests in groups of 5 instead of
firing all pages simultaneously via Promise.all (fixes 429 Too Many
Requests caused by 225 concurrent requests for a 226-page result set)
- Update tests to reflect new permissions endpoint and response shape
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Code Review Agent Run #8888f0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
bot
added
authentication:RBAC
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+153
to
+176
| SupersetClient.get({ | ||
| endpoint: `/api/v1/security/roles/${id}/permissions/`, | ||
| }) | ||
| .then(response => { | ||
| permissionFetchSucceeded.current = true; | ||
| setRolePermissions(data); | ||
| }, | ||
| filters, | ||
| setLoadingState: (loading: boolean) => setLoadingRolePermissions(loading), | ||
| loadingKey: 'rolePermissions', | ||
| addDangerToast, | ||
| errorMessage: t('There was an error loading permissions.'), | ||
| mapResult: (permission: { | ||
| id: number; | ||
| permission: { name: string }; | ||
| view_menu: { name: string }; | ||
| }) => ({ | ||
| value: permission.id, | ||
| label: formatPermissionLabel( | ||
| permission.permission.name, | ||
| permission.view_menu.name, | ||
| ), | ||
| }), | ||
| }); | ||
| }, [addDangerToast, id, stablePermissionIds]); | ||
| setRolePermissions( | ||
| ( | ||
| response.json?.result as Array<{ | ||
| id: number; | ||
| permission_name: string; | ||
| view_menu_name: string; | ||
| }> | ||
| ).map(p => ({ | ||
| value: p.id, | ||
| label: formatPermissionLabel(p.permission_name, p.view_menu_name), | ||
| })), | ||
| ); | ||
| }) | ||
| .catch(() => { | ||
| addDangerToast(t('There was an error loading permissions.')); | ||
| }) | ||
| .finally(() => { | ||
| setLoadingRolePermissions(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: This effect can apply stale results when switching roles quickly because the previous in-flight request is never invalidated. A slower response from the old role can overwrite state for the current role and show incorrect permissions. Add an effect-scoped cancellation flag and guard all state updates/toasts/finally updates so only the latest request updates state. [race condition]
Severity Level: Critical 🚨
- ❌ Role edit modal may display another role's permissions.
- ❌ Saving can assign wrong permissions to selected role.
- ⚠️ Confusing permission labels in Security → List Roles.
Suggested change
| SupersetClient.get({ | |
| endpoint: `/api/v1/security/roles/${id}/permissions/`, | |
| }) | |
| .then(response => { | |
| permissionFetchSucceeded.current = true; | |
| setRolePermissions(data); | |
| }, | |
| filters, | |
| setLoadingState: (loading: boolean) => setLoadingRolePermissions(loading), | |
| loadingKey: 'rolePermissions', | |
| addDangerToast, | |
| errorMessage: t('There was an error loading permissions.'), | |
| mapResult: (permission: { | |
| id: number; | |
| permission: { name: string }; | |
| view_menu: { name: string }; | |
| }) => ({ | |
| value: permission.id, | |
| label: formatPermissionLabel( | |
| permission.permission.name, | |
| permission.view_menu.name, | |
| ), | |
| }), | |
| }); | |
| }, [addDangerToast, id, stablePermissionIds]); | |
| setRolePermissions( | |
| ( | |
| response.json?.result as Array<{ | |
| id: number; | |
| permission_name: string; | |
| view_menu_name: string; | |
| }> | |
| ).map(p => ({ | |
| value: p.id, | |
| label: formatPermissionLabel(p.permission_name, p.view_menu_name), | |
| })), | |
| ); | |
| }) | |
| .catch(() => { | |
| addDangerToast(t('There was an error loading permissions.')); | |
| }) | |
| .finally(() => { | |
| setLoadingRolePermissions(false); | |
| }); | |
| let isActive = true; | |
| SupersetClient.get({ | |
| endpoint: `/api/v1/security/roles/${id}/permissions/`, | |
| }) | |
| .then(response => { | |
| if (!isActive) return; | |
| permissionFetchSucceeded.current = true; | |
| setRolePermissions( | |
| ( | |
| response.json?.result as Array<{ | |
| id: number; | |
| permission_name: string; | |
| view_menu_name: string; | |
| }> | |
| ).map(p => ({ | |
| value: p.id, | |
| label: formatPermissionLabel(p.permission_name, p.view_menu_name), | |
| })), | |
| ); | |
| }) | |
| .catch(() => { | |
| if (isActive) { | |
| addDangerToast(t('There was an error loading permissions.')); | |
| } | |
| }) | |
| .finally(() => { | |
| if (isActive) { | |
| setLoadingRolePermissions(false); | |
| } | |
| }); | |
| return () => { | |
| isActive = false; | |
| }; |
Steps of Reproduction ✅
1. Open the roles list page implemented in
`superset-frontend/src/pages/RolesList/index.tsx` and click the "Edit role" action for a
role A. This triggers `handleEdit` at lines 25–29 (offset block 160–219) which sets
`currentRole` and calls `openModal(ModalType.EDIT)`, causing `<RoleListEditModal
role={currentRole} ... />` to render at lines 31–40 (offset block 300–379).
2. When `RoleListEditModal` mounts, the permissions-loading effect at
`superset-frontend/src/features/roles/RoleListEditModal.tsx:149–177` runs with `id` for
role A. It sets `loadingRolePermissions` to `true` (line 150), resets
`permissionFetchSucceeded.current` (line 151), and issues `SupersetClient.get({ endpoint:
\`/api/v1/security/roles/${id}/permissions/\` })` at lines 153–155 without any
cancellation or staleness guard.
3. Before the request for role A completes (e.g., under high latency), click "Edit role"
on a different role B in the list. `handleEdit` in `RolesList/index.tsx` again sets
`currentRole` to B (lines 25–29) while `modalState.edit` remains true, so the same
`RoleListEditModal` instance re-renders with a new `role.id`. The permissions `useEffect`
(lines 149–177) runs again with the new `id`, issuing a second `SupersetClient.get` for
role B while the original request for role A is still in flight; the first request is not
cancelled.
4. If the request for role B happens to resolve first and the slower request for role A
resolves later, the `.then` handler at lines 156–169 for the A request still executes and
calls `setRolePermissions(...)` and `permissionFetchSucceeded.current = true` based on A's
response, followed by `.finally` at lines 174–175 calling
`setLoadingRolePermissions(false)`. Because there is no `isActive`/cancellation flag, this
late A response overwrites the permissions state for role B, causing the edit modal for
role B to show permissions for role A; when the user saves, `handleFormSubmit` at lines
274–285 uses the incorrect `rolePermissions` state to call `updateRolePermissions(id,
permissionIds)`, persisting mismatched permissions for role B.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/roles/RoleListEditModal.tsx
**Line:** 153:176
**Comment:**
*Race Condition: This effect can apply stale results when switching roles quickly because the previous in-flight request is never invalidated. A slower response from the old role can overwrite state for the current role and show incorrect permissions. Add an effect-scoped cancellation flag and guard all state updates/toasts/finally updates so only the latest request updates state.
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.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Replace `response.json?.result.map()` pattern with null-coalescing fallback to satisfy oxlint no-unsafe-optional-chaining rule. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fixes two related errors on the Role management page when roles have many permissions:
HTTP 414 (URI Too Long) — When loading a role's permissions in the edit modal, the old code built a filter
id in [all_permission_ids]and rison-encoded the entire ID list into the GET query string. For roles with hundreds or thousands of permissions this exceeds URL length limits.Fix: Use the existing FAB endpoint
GET /api/v1/security/roles/{id}/permissions/instead. The role ID goes in the path, so URL length is constant regardless of permission count. One request replaces N paginated requests.HTTP 429 (Too Many Requests) —
fetchPaginatedDatafetched the first page to get the total count, then fired all remaining pages simultaneously viaPromise.all(). With 226 pages that meant 225 concurrent requests at once.Fix: Process pages in sequential batches of 5 (matching the pattern already used in
fetchAllPermissionPagesinutils.ts).Note: #37235 (already merged) fixed the same 414 pattern for the users fetch. This PR applies the same class of fix to permissions.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — fixes network errors, no visual change when working correctly.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION