fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior#38792
fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior#38792jaymasiwal wants to merge 3 commits intoapache:masterfrom
Conversation
Code Review Agent Run #e8bdc8Actionable 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 |
Sequence DiagramThis PR updates how the Select component chooses its popup container to prevent double scrollbars. It now keeps a custom container when provided, otherwise uses the nearest modal container, and falls back to the page body. sequenceDiagram participant User participant Select participant DOM participant Popup User->>Select: Open dropdown Select->>Select: Check custom popup container prop alt Custom container provided Select->>Popup: Render in provided container else No custom container Select->>DOM: Find nearest modal container alt Modal container found Select->>Popup: Render inside modal container else No modal container Select->>Popup: Render in document body end end Generated by CodeAnt AI |
…breaking modal behavior
5eea75e to 1bcce78 Compare Code Review Agent Run #e104aeActionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| await waitFor(async () => { | ||
| const tabs = screen.getAllByText('Specific Date/Time'); | ||
| if (tabs.length < 2) { | ||
| const nows = screen.getAllByText('Now'); | ||
| userEvent.click(nows[nows.length - 1]); | ||
| throw new Error('Waiting for second tab...'); | ||
| } | ||
| userEvent.click(tabs[1]); | ||
| }, { timeout: 3000 }); |
There was a problem hiding this comment.
Suggestion: The waitFor retry callback performs clicks and throws intentionally, which means every retry triggers extra UI side effects and can over-increment onChange unpredictably. waitFor should only poll for a condition, then perform the click once after the condition is met. [logic error]
Severity Level: Major ⚠️
- ⚠️ DateFilterControl mode-change test becomes flaky in CI. - ⚠️ Jest call-count assertion intermittently fails.| await waitFor(async () => { | |
| const tabs = screen.getAllByText('Specific Date/Time'); | |
| if (tabs.length < 2) { | |
| const nows = screen.getAllByText('Now'); | |
| userEvent.click(nows[nows.length - 1]); | |
| throw new Error('Waiting for second tab...'); | |
| } | |
| userEvent.click(tabs[1]); | |
| }, { timeout: 3000 }); | |
| await waitFor( | |
| () => expect(screen.getAllByText('Specific Date/Time')).toHaveLength(2), | |
| { timeout: 3000 }, | |
| ); | |
| const tabs = screen.getAllByText('Specific Date/Time'); | |
| await userEvent.click(tabs[1]); |
Steps of Reproduction ✅
1. Run test `triggers onChange when the mode changes` in `superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:188` with `onChange = jest.fn()`. 2. Execution enters `waitFor` block at `CustomFrame.test.tsx:208-216`; when `tabs.length < 2`, callback clicks a `Now` option (`line 212`) and deliberately throws (`line 213`) to force retry. 3. Each retry repeats the click side effect; in component `superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx:68-74`, `onChange(...)` always calls `props.onChange(...)`, incrementing the mock call count. 4. When retries occur before both tabs appear, extra `onChange` calls are accumulated, making final assertion `toHaveBeenCalledTimes(2)` at `CustomFrame.test.tsx:218` nondeterministic/flaky.Prompt for AI Agent 🤖
This is a comment left during a code review. **Path:** superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx **Line:** 208:216 **Comment:** *Logic Error: The `waitFor` retry callback performs clicks and throws intentionally, which means every retry triggers extra UI side effects and can over-increment `onChange` unpredictably. `waitFor` should only poll for a condition, then perform the click once after the condition is met. 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.a0b917d to da8536f Compare | All CI checks are now passing. I have updated the test selectors in SelectControl.test.tsx and CustomFrame.test.tsx to use global screen and async findBy logic to support the transition to Portals. This PR is ready for final review and merge. Thanks! |
Code Review Agent Run #23c825Actionable 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 |
| Hi everyone, I’ve been working on improving frontend UI behavior in Superset, particularly around dropdown rendering and container handling. Recent contributions:
While working on these, I encountered recurring issues related to:
I’ve also worked through fixing related Jest test failures (especially around portal-based dropdowns and async rendering), which highlighted gaps in current testing patterns. I’m planning to propose a GSoC (Google Summer of Code 26) project focused on:
Before finalizing the proposal, I wanted to ask: Would really appreciate any feedback. Thanks! |
| @jaymasiwal can you add before and after screenshots? |
| Also is this a duplicate of #38659? if so can you either close one or combine them into one PR? |
User description
Fixes double scrollbar issue in Select dropdown without breaking existing behavior.
.ant-modaldocument.bodyotherwisePrevents nested scroll containers while maintaining correct dropdown positioning.
CodeAnt-AI Description
Keep Select dropdowns inside modals without adding an extra scrollbar
What Changed
Impact
✅ Fewer double-scrollbar issues in modal dropdowns✅ Cleaner Select behavior inside dialogs✅ Stable dropdown placement outside modals💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.