fix(UI): remove getPopupContainer prop causing dual scrollbars in dropdown#36059
fix(UI): remove getPopupContainer prop causing dual scrollbars in dropdown#36059shivamsahu-tech wants to merge 1 commit intoapache:masterfrom
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx | ✅ |
| superset-frontend/src/features/databases/DatabaseModal/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
shivamsahu-tech
changed the title
shivamsahu-tech
changed the title
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #d4a06f
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx - 1
- Missing getPopupContainer prop in Select component · Line 734-739
Review Details
-
Files reviewed - 2 · Commit Range:
c500faa..c500faa- superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
- superset-frontend/src/features/databases/DatabaseModal/index.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 Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| popupRender={popupRender} | ||
| filterOption={handleFilterOption} | ||
| filterSort={sortComparatorWithSearch} | ||
| getPopupContainer={ | ||
| getPopupContainer || (triggerNode => triggerNode.parentNode) | ||
| } | ||
| headerPosition={headerPosition} | ||
| labelInValue={labelInValue} | ||
| maxTagCount={actualMaxTagCount} |
There was a problem hiding this comment.
The getPopupContainer prop was removed from StyledSelect, creating inconsistency with AsyncSelect.tsx where it's still passed. This could break custom popup positioning if the prop is used. Add it back to ensure consistent behavior across Select components.
Code suggestion
Check the AI-generated fix before applying
| popupRender={popupRender} | |
| filterOption={handleFilterOption} | |
| filterSort={sortComparatorWithSearch} | |
| getPopupContainer={ | |
| getPopupContainer || (triggerNode => triggerNode.parentNode) | |
| } | |
| headerPosition={headerPosition} | |
| labelInValue={labelInValue} | |
| maxTagCount={actualMaxTagCount} | |
| popupRender={popupRender} | |
| filterOption={handleFilterOption} | |
| filterSort={sortComparatorWithSearch} | |
| getPopupContainer={ | |
| getPopupContainer || (triggerNode => triggerNode.parentNode) | |
| } | |
| headerPosition={headerPosition} | |
| labelInValue={labelInValue} | |
| maxTagCount={actualMaxTagCount} |
Code Review Run #d4a06f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a dual scrollbar issue in dropdown menus by removing the getPopupContainer prop from two locations in the codebase. The prop was forcing dropdowns to render within constrained parent containers, causing scrolling issues and visual clipping.
Key changes:
- Removed
getPopupContainerprop from the Select component insuperset-ui-core - Removed
getPopupContainerprop from the DatabaseModal component - Dropdowns now render to
document.bodyby default (Ant Design's standard behavior)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx |
Removed default getPopupContainer implementation that was forcing dropdowns to render in parent nodes |
superset-frontend/src/features/databases/DatabaseModal/index.tsx |
Removed getPopupContainer prop from database selector dropdown to prevent dual scrollbars |
| popupRender={popupRender} | ||
| filterOption={handleFilterOption} | ||
| filterSort={sortComparatorWithSearch} |
There was a problem hiding this comment.
The getPopupContainer prop was removed here to fix the dual scrollbar issue, but it's still present in AsyncSelect.tsx (lines 611-613). For consistency and to ensure the same issue doesn't occur with async select dropdowns, consider removing it from AsyncSelect as well. This would ensure both Select components have consistent behavior and avoid potential dual scrollbar issues in async select dropdowns.
| popupRender={popupRender} | ||
| filterOption={handleFilterOption} | ||
| filterSort={sortComparatorWithSearch} |
There was a problem hiding this comment.
The getPopupContainer prop is still defined in the TypeScript interface AntdExposedProps (line 72 in types.ts), but it's no longer being used in the implementation. This creates a type/implementation mismatch where users can pass the prop without TypeScript errors, but it will be silently ignored.
Consider removing getPopupContainer from the AntdExposedProps type definition to match the implementation and prevent confusion.
|
@geido @kgabryje @msyavuz If there is a preferred or more correct way to handle this, I’m ready to update the implementation accordingly. I had commented the whole solution approach in the issue section earlier, before this PR, but didn’t receive a response for a few weeks. Due to many contributors’ comments, I went ahead and opened this PR. I’m a student, so if I am missing anything, sorry for that. |
|
Hey @shivamsahu-tech, copilot is mostly correct on this and the fix should probably be scoped to that dataset select component. Select is used all around and removing that getPopupContainer prop passthrough would break a lot of other things. Thanks for opening a pr and taking care of this! |
shivamsahu-tech
force-pushed
the
fix/issue-35833-dropdown-dual-scrollbar
branch
from
5b4038f to
4fa1b1b
Compare
|
Hey @msyavuz , Sorry for the previous inconsistent changes. In this new fix, I’ve only updated the "DatabaseModal/index.tsx" file, only modifying the value of the "getPopupContainer" prop. The change is scoped to this component only. Tested locally, it works fine. I think everything should be in order now. If there’s any issue, please let me know, I’m happy to learn and improve as a student. |
|
Hey @msyavuz @kgabryje , Here .ant-select-dropdown was getting height: 160px (visible in browser dev tools) Adding only height: 'auto' to the dropdownStyle prop allows the parent container to expand and accommodate the child's size instead of being constrained to 160px, that resolve the dual scrollbar issue, tested locally. Changes: // OLD
dropdownStyle={{ maxHeight: 400, overflow: 'auto' }}
// NEW
dropdownStyle={{ maxHeight: 400, overflow: 'auto', height: 'auto' }}This is a minimal change (one additional property) and shouldn't affect anything else. |
|
I reproduced this locally on the latest master and confirmed that setting height: auto (or removing the fixed height) resolves the double scrollbar issue without regressions in the other dropdowns I tested. So yeah this confirms that the fix proposed in #36059 addresses the issue. Happy to help with further testing or validation if needed. |
fix(UI): remove getPopupContainer prop causing dual scrollbars in dropdown menus (#35833)
SUMMARY
This PR fixes the dual scrollbar issue appearing in dropdown menus.
The issue occurred because the
getPopupContainerprop was being passed to the Ant Design Select component in both:superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsxsuperset-frontend/src/features/databases/DatabaseModal/index.tsxThis prop forced dropdowns to render inside their parent container rather than the default
document.body.Since these parent containers have constrained height and overflow properties, the dropdown scrolled within the parent component, resulting in dual scrollbars and clipped dropdowns.
Edit: To avoid inconsistency, the latest changes only update the DatabaseModal/index.tsx file, modifying the getPopupContainer prop value while ensuring nothing else breaks.
BEFORE/AFTER SCREENSHOTS
Before (with issue)
After (fixed)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION