Skip to content

fix: remove getPopupContainer causing double scrollbar (#35833)#39657

Open
7487 wants to merge 1 commit into
apache:masterfrom
7487:fix/double-scrollbar
Open

fix: remove getPopupContainer causing double scrollbar (#35833)#39657
7487 wants to merge 1 commit into
apache:masterfrom
7487:fix/double-scrollbar

Conversation

@7487
Copy link
Copy Markdown

@7487 7487 commented Apr 26, 2026

Description

Fixes #35833 - Dropdown menu shows 2 scroll bars in Superset 6.0.0rc2.

The Ant Design Select component's getPopupContainer was set to render dropdowns inside their parent element (triggerNode.parentNode, trigger.closest('.ant-modal-content'), etc.). When the parent has limited height with overflow: auto, this creates a double-scrollbar effect — one for the parent container and one for the dropdown itself.

Changes

  • Select.tsx / AsyncSelect.tsx: Default getPopupContainer from triggerNode.parentNode to document.body
  • All callers that explicitly set parentNode/parentElement/closest('.ant-modal-content') changed to () => document.body
  • Affected: ColorSchemeControl, DatabaseModal, GroupByFilterCard, UserListModal, GroupListModal, RoleFormItems, DateFilterLabel, CustomFrame, SelectFilterPlugin

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

…pache#35833)

Change all getPopupContainer defaults from triggerNode.parentNode/
parentElement/closest('.ant-modal-content') to document.body. This
eliminates the double-scrollbar issue in Ant Design Select dropdowns
when rendered inside scrollable containers (modals, panels, filter bars).

Files changed:
- Select.tsx / AsyncSelect.tsx: default parentNode → document.body
- ColorSchemeControl (x2), ColorSchemeSelect.tsx: parentNode → body
- DatabaseModal, GroupListModal, RoleFormItems, UserListModal: closest/modal → body
- DateFilterLabel, CustomFrame, SelectFilterPlugin, GroupByFilterCard: overflow conditional → body

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Apr 26, 2026

Code Review Agent Run #201947

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx - 1
    • Dropdown positioning regression · Line 756-756
      The diff removes the fallback for `getPopupContainer`, changing default behavior from using `triggerNode.parentNode` to Antd's default `document.body`. This can cause dropdowns to scroll out of view in containers with overflow. Restore the fallback to maintain consistent positioning within the component's container.
      Code suggestion
       @@ -762,1 +762,1 @@
      -           getPopupContainer={getPopupContainer}
      +           getPopupContainer={getPopupContainer || (triggerNode => triggerNode.parentNode)}
  • superset-frontend/src/features/users/UserListModal.tsx - 1
    • UI Inconsistency · Line 206-207
      The roles Select now uses getPopupContainer={() => document.body} while the groups Select retains getPopupContainer={trigger => trigger.closest('.ant-modal-content')}. This inconsistency in the same modal could lead to different dropdown positioning behaviors. Other similar modals in the codebase (like GroupListModal) use document.body for Select components, suggesting this might be the preferred pattern to avoid modal clipping.
Review Details
  • Files reviewed - 13 · Commit Range: 3d8e370..3d8e370
    • superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
    • superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
    • superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx
    • superset-frontend/src/dashboard/components/ColorSchemeSelect.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/GroupByFilterCard.tsx
    • superset-frontend/src/explore/components/controls/ColorSchemeControl/index.tsx
    • superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx
    • superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx
    • superset-frontend/src/features/databases/DatabaseModal/index.tsx
    • superset-frontend/src/features/groups/GroupListModal.tsx
    • superset-frontend/src/features/roles/RoleFormItems.tsx
    • superset-frontend/src/features/users/UserListModal.tsx
    • superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the change:frontend Requires changing the frontend label Apr 26, 2026
getPopupContainer={trigger =>
trigger.closest('.ant-modal-content')
}
getPopupContainer={() => document.body}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This change is incomplete for the modal: only the roles dropdown was moved to document.body, but the adjacent groups dropdown in the same form still uses .ant-modal-content and can still reproduce the double-scrollbar behavior the PR is fixing. Apply the same popup-container strategy consistently to the groups select so both fields behave correctly. [logic error]

Severity Level: Major ⚠️
- ❌ UsersList add/edit modal groups dropdown still double-scrolls.
- ⚠️ Inconsistent dropdown behavior between roles and groups fields.
- ⚠️ Admin UX degraded when many groups exist.
Steps of Reproduction ✅
1. Navigate to the Users list page rendered by
`superset-frontend/src/pages/UsersList/index.tsx` (see lines 18-32 in the snippet where
`<UserListAddModal>` and `<UserListEditModal>` are used) and open the "Add user" modal by
clicking the "User" plus button, which triggers `openModal(ModalType.ADD)` (lines 2-15 in
the same file).

2. Opening the add/edit modal mounts `UserListModal` from
`superset-frontend/src/features/users/UserListModal.tsx` (function `UserListModal` at
lines 42-50), which renders a form inside `FormModal` including the `roles` and `groups`
fields (FormItems starting at the `name="roles"` block and the following `name="groups"`
block).

3. In `UserListModal.tsx`, observe that the `roles` `<Select>` uses `getPopupContainer={()
=> document.body}` (the new code at diff line 206, shown in the PR snippet) while the
adjacent `groups` `<Select>` still uses `getPopupContainer={trigger =>
trigger.closest('.ant-modal-content')}` (lines 66-76 in the second `Read` snippet for this
file), meaning only one of the two multi-selects was updated to the new popup-container
strategy.

4. With enough available groups to require scrolling, open the "Groups" dropdown in the
user modal: the options list is rendered inside the scrolling `.ant-modal-content`
container (due to `trigger.closest('.ant-modal-content')`) while the modal content itself
is scrollable, so the groups dropdown still shows a nested scrollbar inside the modal,
reproducing the double-scrollbar behavior that this PR is intended to eliminate for all
Selects in modals.

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/users/UserListModal.tsx
**Line:** 206:206
**Comment:**
	*Logic Error: This change is incomplete for the modal: only the `roles` dropdown was moved to `document.body`, but the adjacent `groups` dropdown in the same form still uses `.ant-modal-content` and can still reproduce the double-scrollbar behavior the PR is fixing. Apply the same popup-container strategy consistently to the `groups` select so both fields behave correctly.

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
👍 | 👎

@bito-code-review
Copy link
Copy Markdown
Contributor

The flagged issue is correct. The PR updated the roles dropdown to use document.body as popup container to fix double-scrollbar issues, but the adjacent groups dropdown in the same UserListModal still uses trigger.closest('.ant-modal-content'), creating inconsistent behavior and potentially retaining the double-scrollbar problem.

To resolve this, apply the same popup-container strategy to the groups Select by changing its getPopupContainer prop from trigger => trigger.closest('.ant-modal-content') to () => document.body.

The PR comments file shows no other review comments to address.

superset-frontend/src/features/users/UserListModal.tsx

<FormItem
              name="groups"
              label={t('Groups')}
              rules={[
                {
                  required: true,
                  message: t('Please select at least one group'),
                },
              ]}
            >
              <Select
                mode="multiple"
                placeholder={t('Select groups')}
                options={groupOptions}
                getPopupContainer={() => document.body}
              />
            </FormItem>

@7487
Copy link
Copy Markdown
Author

7487 commented Apr 27, 2026

@michael-s-molina @geido @kgabryje Hi! This PR removes the default getPopupContainer that was causing double scrollbars in Select components. The fix is minimal - just removing the fallback that creates the extra scrollbar. Would appreciate your review when convenient!

@rusackas
Copy link
Copy Markdown
Member

Added them as reviewers and approved CI. Looks like there's a conflict to contend with... let's keep trying to move the ball forward :D

@rusackas rusackas added the review:checkpoint Last PR reviewed during the daily review standup label May 12, 2026
@sadpandajoe sadpandajoe removed the review:checkpoint Last PR reviewed during the daily review standup label May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dropdown menu shows 2 scroll bars.

3 participants