fix(rbac): widen permissions dropdown in role form#40472
Conversation
Code Review Agent Run #611179Actionable 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 |
| fetchPermissionOptions(filterValue, page, pageSize, addDangerToast) | ||
| } | ||
| loading={loading} | ||
| getPopupContainer={trigger => trigger.closest('.ant-modal-content')} |
There was a problem hiding this comment.
Suggestion: getPopupContainer must always return an HTMLElement, but closest('.ant-modal-content') can return null. When that happens, the select popup mounting path can fail at runtime; add a fallback container (for example parent element or document.body) so the callback never returns null. [null pointer]
Severity Level: Major ⚠️
- ❌ Permissions select dropdown can crash when modal container missing.
- ⚠️ Role create/edit UI becomes fragile to DOM changes.Steps of Reproduction ✅
1. Open the Roles list page implemented in
`superset-frontend/src/pages/RolesList/index.tsx:320-359`, and trigger either the "+ Role"
add modal (`RoleListAddModal`) or the "Edit Role" modal (`RoleListEditModal`) by clicking
the corresponding action in the UI; both modals are rendered from this page.
2. In the Add Role flow, `RoleListAddModal` at
`superset-frontend/src/features/roles/RoleListAddModal.tsx:31-52` renders
`<PermissionsField addDangerToast={addDangerToast} />`, and in the Edit Role flow,
`RoleListEditModal` at
`superset-frontend/src/features/roles/RoleListEditModal.tsx:104-121,358-361` renders
`<PermissionsField addDangerToast={addDangerToast} loading={loadingRolePermissions} />`,
so the `PermissionsField` component is mounted inside an Ant Design `FormModal`.
3. `PermissionsField` is defined in
`superset-frontend/src/features/roles/RoleFormItems.tsx:44-62` and renders an
`AsyncSelect` with `getPopupContainer={trigger => trigger.closest('.ant-modal-content')}`
at line 57; when the user clicks the permissions dropdown, `AsyncSelect` forwards this
callback to its internal `StyledSelect` (see
`packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:20-27` where
`getPopupContainer` is passed through).
4. At runtime, `StyledSelect` ultimately passes `getPopupContainer` down to the base
`Select` component (`packages/superset-ui-core/src/components/Select/Select.tsx:17-19`),
which expects a function returning an `HTMLElement`; however, the DOM API
`Element.closest()` used in `trigger.closest('.ant-modal-content')` is defined to return
`Element | null`, so in any situation where the trigger node has no ancestor matching
`.ant-modal-content` (for example if the modal markup changes or `PermissionsField` is
reused outside an Ant Design modal), this callback returns `null`, causing the Select's
popup mounting logic to receive a non-HTMLElement container and potentially throw at
runtime. Other call sites in this codebase (e.g. `ChartHolder` at
`src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:9-15` and
`AsyncSelect`'s default `triggerNode => triggerNode.parentNode` at
`AsyncSelect.tsx:25-27`) explicitly ensure a non-null fallback (often `document.body`),
confirming that always returning an `HTMLElement` is the intended and safer pattern.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/RoleFormItems.tsx
**Line:** 57:57
**Comment:**
*Null Pointer: `getPopupContainer` must always return an `HTMLElement`, but `closest('.ant-modal-content')` can return `null`. When that happens, the select popup mounting path can fail at runtime; add a fallback container (for example parent element or `document.body`) so the callback never returns null.
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|
The suggestion is valid and should be applied. The current implementation uses superset-frontend/src/features/roles/RoleFormItems.tsx |
Code Review Agent Run #1f10a3Actionable 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 |
SUMMARY
Fixes #40430.
This updates the permissions AsyncSelect dropdown in the role create/edit form so long permission names are easier to read.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
npm run lintADDITIONAL INFORMATION
Note: local
npm run typesurfaced unrelated plugin workspace TypeScript issues inplugin-chart-word-cloudandpreset-chart-deckgl, not caused by this change.