codeant-ai-for-open-source[bot] commented on code in PR #40472:
URL: https://github.com/apache/superset/pull/40472#discussion_r3312818298
##########
superset-frontend/src/features/roles/RoleFormItems.tsx:
##########
@@ -47,16 +47,17 @@ export const PermissionsField = ({
}: AsyncOptionsFieldProps) => (
<FormItem name="rolePermissions" label={t('Permissions')}>
<AsyncSelect
- mode="multiple"
- name="rolePermissions"
- placeholder={t('Select permissions')}
- options={(filterValue, page, pageSize) =>
- fetchPermissionOptions(filterValue, page, pageSize, addDangerToast)
- }
- loading={loading}
- getPopupContainer={trigger => trigger.closest('.ant-modal-content')}
- data-test="permissions-select"
- />
+ mode="multiple"
+ name="rolePermissions"
+ placeholder={t('Select permissions')}
+ options={(filterValue, page, pageSize) =>
+ fetchPermissionOptions(filterValue, page, pageSize, addDangerToast)
+ }
+ loading={loading}
+ getPopupContainer={trigger => trigger.closest('.ant-modal-content')}
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Permissions select dropdown can crash when modal container missing.
- ⚠️ Role create/edit UI becomes fragile to DOM changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b55f20b10b9b481cb1f34de0a500c9e2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b55f20b10b9b481cb1f34de0a500c9e2&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/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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40472&comment_hash=ca0d649d015ebd4f067908e3ed865d3c342fedbaaa0a3f2fc15aac0a3b634bb7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40472&comment_hash=ca0d649d015ebd4f067908e3ed865d3c342fedbaaa0a3f2fc15aac0a3b634bb7&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]
