You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jaymasiwal (via GitHub)" <gi...@apache.org> on 2026/03/22 15:48:24 UTC
[PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
jaymasiwal opened a new pull request, #38792:
URL: https://github.com/apache/superset/pull/38792
Fixes double scrollbar issue in Select dropdown without breaking existing behavior.
- Preserves getPopupContainer if provided
- Falls back to modal container when inside `.ant-modal`
- Defaults to `document.body` otherwise
Prevents nested scroll containers while maintaining correct dropdown positioning.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4185550654
I tried to de-noise the thread of bot-junk a little bit, but it looks like there are a lot of AI review comments that might not have been addressed yet.
As for the GSoC proposals, I think @villebro and @betodealmeida are open to these being submitted through ASF channels, but we probably shouldn't convolute this thread with other discussions since it's already a long one :D I'll just say that we're open to more imrpovements, GSoC or otherwise. The availability to process proposals or provide mentorship is up to those volunteering their time for the official effort.
Once the PR feedback suggestions are implemented or refuted (whichever is the case) we'll try to give this a final review and merge it!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4129534509
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38792?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.61%. Comparing base ([`16f5a2a`](https://app.codecov.io/gh/apache/superset/commit/16f5a2a41a66643c51c727f0f43dd2620004e4bf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e022ca2`](https://app.codecov.io/gh/apache/superset/commit/e022ca2179c45786af5e4fabef6c9c75cf27c6da?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/38792?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/lint\_metadata.py](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Flint_metadata.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2xpbnRfbWV0YWRhdGEucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/reports/notifications/slack\_mixin.py](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&filepath=superset%2Freports%2Fnotifications%2Fslack_mixin.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrX21peGluLnB5) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/mock\_data.py](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&filepath=superset%2Futils%2Fmock_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/38792?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.85%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #38792 +/- ##
==========================================
- Coverage 65.54% 63.61% -1.94%
==========================================
Files 1823 2536 +713
Lines 73154 130677 +57523
Branches 23437 30301 +6864
==========================================
+ Hits 47951 83129 +35178
- Misses 25203 46062 +20859
- Partials 0 1486 +1486
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/38792/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/38792/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.38% <20.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/38792/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.39% <20.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/38792/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.14% <40.00%> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/38792/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.02% <40.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/38792?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4129265105
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4635399134
Thanks @jaymasiwal! The `Select.tsx` change itself looks like the right direction. Two things before this can go in:
The big one: this PR deletes ~2,500 lines of tests — `SelectControl` (474→189), `CustomFrame` (342→204), and most of `DatasetList.listview` — and a lot of that (delete/bulk/permission/error-path tests) has nothing to do with dropdowns. The portal change means some selectors need updating to `screen`/`within(document.body)`, but let's update them rather than remove the cases. Could you restore those and keep this scoped to the actual fix?
Smaller: `AsyncSelect.tsx` still has the old `parentNode` default — should it get the same treatment so the two stay consistent? And the modal selector elsewhere in the codebase is `.ant-modal-content` rather than `.ant-modal`.
Then a rebase (heads up: `DatasetList.listview.test.tsx` will conflict — master's version is the one to keep). Happy to re-review once it's slimmed down
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r2986421738
##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:
##########
@@ -182,20 +184,37 @@ test('triggers onChange when the value changes', async () => {
expect(onChange).toHaveBeenCalled();
});
+
test('triggers onChange when the mode changes', async () => {
const onChange = jest.fn();
render(<CustomFrame onChange={onChange} value={todayNowValue} />, {
store,
});
await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
- userEvent.click(screen.getByTitle('Midnight'));
- expect(await screen.findByTitle('Relative Date/Time')).toBeInTheDocument();
- userEvent.click(screen.getByTitle('Relative Date/Time'));
- userEvent.click(screen.getAllByTitle('Now')[1]);
+
+ const midnightOptions = await screen.findAllByTitle('Midnight');
+ userEvent.click(midnightOptions[0]);
+
+ const relativeOptions = await screen.findAllByTitle('Relative Date/Time');
+ userEvent.click(relativeOptions[0]);
+
+ const nowOptions = await screen.findAllByTitle('Now');
+ userEvent.click(nowOptions[0]);
+
expect(
await screen.findByText('Configure custom time range'),
).toBeInTheDocument();
- userEvent.click(screen.getAllByTitle('Specific Date/Time')[1]);
+
+ 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 });
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ DateFilterControl mode-change test becomes flaky in CI.
- ⚠️ Jest call-count assertion intermittently fails.
```
</details>
```suggestion
await waitFor(
() => expect(screen.getAllByText('Specific Date/Time')).toHaveLength(2),
{ timeout: 3000 },
);
const tabs = screen.getAllByText('Specific Date/Time');
await userEvent.click(tabs[1]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=3f4d8328897f14f7e32a30995c33fb55a0e6dcad62529e0188bb99fcb59311c9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=3f4d8328897f14f7e32a30995c33fb55a0e6dcad62529e0188bb99fcb59311c9&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4124745126
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!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4776764728
@rusackas Just a quick ping! I've addressed all the feedback from your last review (reverted the license headers back to 3-space and restored OAuth2ClientField). The CI pipeline is now completely green. Whenever you have a moment, it is ready for your final sign-off. Thanks again for the guidance!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4124789033
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4757408782
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4106489027
## **Sequence Diagram**
This 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.
```mermaid
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](https://codeant.ai)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r2990889273
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -523,30 +523,33 @@ const DashboardBuilder = () => {
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
{dropIndicatorProps && <div {...dropIndicatorProps} />}
- {!isReport && topLevelTabs && !uiConfig.hideTab && !uiConfig.hideNav && (
- <WithPopoverMenu
- shouldFocus={shouldFocusTabs}
- menuItems={[
- <IconButton
- key="collapse-tabs"
- icon={<Icons.FallOutlined iconSize="xl" />}
- label={t('Collapse tab content')}
- onClick={handleDeleteTopLevelTabs}
- />,
- ]}
- editMode={editMode}
- >
- <DashboardComponent
- id={topLevelTabs?.id}
- parentId={DASHBOARD_ROOT_ID}
- depth={DASHBOARD_ROOT_DEPTH + 1}
- index={0}
- renderTabContent={false}
- renderHoverMenu={false}
- onChangeTab={handleChangeTab}
- />
- </WithPopoverMenu>
- )}
+ {!isReport &&
+ topLevelTabs &&
+ !uiConfig.hideTab &&
+ !uiConfig.hideNav && (
Review Comment:
**Suggestion:** The new condition incorrectly ties tab visibility to `hideNav`. In this codebase, `hideTab` is the dedicated flag for tab visibility, while `hideNav` is used for global navigation (for example, the home menu). This causes dashboards with `hideNav=true` and `hideTab=false` to lose their top-level tabs unexpectedly, breaking embedded/dashboard navigation behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded dashboards can lose top-level tab navigation.
- ⚠️ `uiConfig=4` hides tabs despite `hideTab=false`.
```
</details>
```suggestion
!uiConfig.hideTab && (
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard with root-level tabs (rendered by `DashboardBuilder`), where tab
rendering is driven by `renderDraggableContent` in
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:522-553`,
and used by `<Droppable>` at `:635-650`.
2. Pass URL query `uiConfig=4` (bitmask parsed in `EmbeddedUiConfigProvider`), which sets
`hideNav` true via `hideNav: (config & 4) !== 0` in
`superset-frontend/src/components/UiConfigContext/index.tsx:51-56`.
3. `DashboardBuilder` reads `useUiConfig()` at
`superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:360` and
evaluates the condition `!uiConfig.hideNav` at `:529`.
4. Observe top-level tabs are not rendered even when `hideTab` is false, because the new
condition blocks `<DashboardComponent id={topLevelTabs?.id} .../>` in
`DashboardBuilder.tsx:542-550`, removing in-dashboard tab navigation.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
**Line:** 528:529
**Comment:**
*Logic Error: The new condition incorrectly ties tab visibility to `hideNav`. In this codebase, `hideTab` is the dedicated flag for tab visibility, while `hideNav` is used for global navigation (for example, the home menu). This causes dashboards with `hideNav=true` and `hideTab=false` to lose their top-level tabs unexpectedly, breaking embedded/dashboard navigation behavior.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=7ba894a6434db929d709828072f5882deaebde8f0709d0397ff7c26b75ead18b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=7ba894a6434db929d709828072f5882deaebde8f0709d0397ff7c26b75ead18b&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:
##########
@@ -188,14 +188,32 @@ test('triggers onChange when the mode changes', async () => {
store,
});
await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
- userEvent.click(screen.getByTitle('Midnight'));
- expect(await screen.findByTitle('Relative Date/Time')).toBeInTheDocument();
- userEvent.click(screen.getByTitle('Relative Date/Time'));
- userEvent.click(screen.getAllByTitle('Now')[1]);
+
+ const midnightOptions = await screen.findAllByTitle('Midnight');
+ userEvent.click(midnightOptions[0]);
+
+ const relativeOptions = await screen.findAllByTitle('Relative Date/Time');
+ userEvent.click(relativeOptions[0]);
+
+ const nowOptions = await screen.findAllByTitle('Now');
+ userEvent.click(nowOptions[0]);
+
expect(
await screen.findByText('Configure custom time range'),
).toBeInTheDocument();
- userEvent.click(screen.getAllByTitle('Specific Date/Time')[1]);
+
+ // Polling condition ONLY - No side effects (clicks) inside here
+ await waitFor(
+ () => {
+ expect(screen.getAllByText('Specific Date/Time')).toHaveLength(2);
+ },
+ { timeout: 3000 },
+ );
+
+ // Perform the click ONCE after the condition is met
+ const tabs = screen.getAllByText('Specific Date/Time');
+ userEvent.click(tabs[1]);
Review Comment:
**Suggestion:** The test is selecting options via `findAllByTitle(...)[index]`, which is unstable with portal-rendered Ant Select dropdowns because hidden/stale nodes can match and the wrong element can be clicked. This can make the mode never change and cause intermittent failures/timeouts. Target the specific combobox and choose visible options by role instead of title/index. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `CustomFrame` mode-change test can be flaky in CI.
- ⚠️ Intermittent failures may block frontend PR merges.
```
</details>
```suggestion
const startModeSelect = screen.getByRole('combobox', {
name: 'Start (inclusive)',
});
userEvent.click(startModeSelect);
userEvent.click(
await screen.findByRole('option', { name: 'Relative Date/Time' }),
);
const endModeSelect = screen.getByRole('combobox', {
name: 'End (exclusive)',
});
userEvent.click(endModeSelect);
userEvent.click(
await screen.findByRole('option', { name: 'Specific Date/Time' }),
);
expect(
await screen.findByText('Configure custom time range'),
).toBeInTheDocument();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run test `triggers onChange when the mode changes` in
`superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:185`
so execution enters the new sequence at lines 192-215.
2. The test queries globally with `screen.findAllByTitle(...)` and clicks index 0
(`CustomFrame.test.tsx:192-200`), which is not scoped to a specific select and depends on
DOM ordering.
3. `CustomFrame` renders two `Select` controls (`CustomFrame.tsx:130-135` and `189-194`)
with overlapping option labels, while `Select` portals dropdowns via `getPopupContainer`
(`packages/superset-ui-core/src/components/Select/Select.tsx:180-183`), so title-based
global matches can include non-target nodes.
4. When index 0 resolves to a non-target match, the expected mode transition is not
applied; downstream assertions can fail (`waitFor` at `CustomFrame.test.tsx:206-211` or
`toHaveBeenCalledTimes(2)` at line 217), causing intermittent test instability.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx
**Line:** 192:215
**Comment:**
*Logic Error: The test is selecting options via `findAllByTitle(...)[index]`, which is unstable with portal-rendered Ant Select dropdowns because hidden/stale nodes can match and the wrong element can be clicked. This can make the mode never change and cause intermittent failures/timeouts. Target the specific combobox and choose visible options by role instead of title/index.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=99cea0ce05fd19971c44437158ed28e6e7918b87a8537d2d78000f2759e4e17c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=99cea0ce05fd19971c44437158ed28e6e7918b87a8537d2d78000f2759e4e17c&reaction=dislike'>👎</a>
##########
superset/databases/api.py:
##########
@@ -630,8 +630,9 @@ def delete(self, pk: int) -> Response:
@protect()
@statsd_metrics
@event_logger.log_this_with_context(
- action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
- f".sync-permissions",
+ action=lambda self, *args, **kwargs: (
+ f"{self.__class__.__name__}.sync-permissions"
Review Comment:
**Suggestion:** The new event action name uses a hyphen (`sync-permissions`) while this API and the rest of the codebase use underscore-based action names. This changes the emitted audit event key from the previous default (`sync_permissions`) and can break consumers that filter/group logs by the existing action contract. Keep the action value aligned with the method name to preserve compatibility. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ sync_permissions audit action key changes unexpectedly.
- ⚠️ Existing action-based log filters can miss events.
```
</details>
```suggestion
f"{self.__class__.__name__}.sync_permissions"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call `POST /api/v1/database/<pk>/sync_permissions/`, which is routed by
`sync_permissions()` at `superset/databases/api.py:629-638`.
2. The decorator at `superset/databases/api.py:632-637` forces action to
`DatabaseRestApi.sync-permissions`.
3. In `superset/utils/log.py:28-30`, `_wrapper()` uses that explicit `action` instead of
default `f.__name__` (which would be `sync_permissions`).
4. `DBEventLogger.log()` persists this value into `Log.action` at
`superset/utils/log.py:135-137` (`Log` model field defined at
`superset/models/core.py:1369`).
5. Query log views/searches that filter by action (supported in
`superset/views/log/api.py:61-64`) and observe entries now use hyphenated key, so
underscore-based filters/contracts miss these events.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/databases/api.py
**Line:** 634:634
**Comment:**
*Logic Error: The new event action name uses a hyphen (`sync-permissions`) while this API and the rest of the codebase use underscore-based action names. This changes the emitted audit event key from the previous default (`sync_permissions`) and can break consumers that filter/group logs by the existing action contract. Keep the action value aligned with the method name to preserve compatibility.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=9948e076681fa71f6bbba910f7c3b32ddafedc50ab283a9f75398f161c6e916e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=9948e076681fa71f6bbba910f7c3b32ddafedc50ab283a9f75398f161c6e916e&reaction=dislike'>👎</a>
##########
superset/reports/notifications/slack_mixin.py:
##########
@@ -94,7 +94,7 @@ def _get_body(self, content: NotificationContent) -> str:
# need to truncate the data
for i in range(len(df) - 1):
truncated_df = df[: i + 1].fillna("")
- truncated_row = pd.Series({k: "..." for k in df.columns})
+ truncated_row = pd.Series(dict.fromkeys(df.columns, "..."))
Review Comment:
**Suggestion:** Building `truncated_row` with `dict.fromkeys(df.columns, "...")` drops duplicate column names because dict keys must be unique. If the dataframe has duplicate columns (which can happen after flattening tuple headers), the subsequent `pd.concat` raises `InvalidIndexError` at runtime. Build the series with an explicit index so duplicates are preserved. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slack TEXT reports can fail before sending.
- ⚠️ Affects Slack v1 and v2 notification paths.
```
</details>
```suggestion
truncated_row = pd.Series(["..."] * len(df.columns), index=df.columns)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a TEXT report execution path where `NotificationContent.embedded_data` is
populated (`superset/commands/report/execute.py:642`, `_get_notification_content` calling
`_get_embedded_data`).
2. Ensure the dataframe has duplicate column labels after flattening in
`SlackMixin._get_body` (`superset/reports/notifications/slack_mixin.py:75-83`), e.g.
duplicate names from chart metadata.
3. Send Slack notification through `SlackNotification.send`
(`superset/reports/notifications/slack.py:91`) or `SlackV2Notification.send`
(`superset/reports/notifications/slackv2.py:87`), both calling `_get_body`.
4. Execution reaches `truncated_row = pd.Series(dict.fromkeys(df.columns, "..."))`
(`superset/reports/notifications/slack_mixin.py:97`), which drops duplicate keys.
5. Next `pd.concat` (`superset/reports/notifications/slack_mixin.py:98-99`) raises
`InvalidIndexError`; verified with pandas runtime reproduction (`Reindexing only valid
with uniquely valued Index objects`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/slack_mixin.py
**Line:** 97:97
**Comment:**
*Type Error: Building `truncated_row` with `dict.fromkeys(df.columns, "...")` drops duplicate column names because dict keys must be unique. If the dataframe has duplicate columns (which can happen after flattening tuple headers), the subsequent `pd.concat` raises `InvalidIndexError` at runtime. Build the series with an explicit index so duplicates are preserved.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=badea1f813b564281ec0da86f0b2c5f8f6cad465a574ad464e46e814fc30fcfa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=badea1f813b564281ec0da86f0b2c5f8f6cad465a574ad464e46e814fc30fcfa&reaction=dislike'>👎</a>
##########
tests/unit_tests/async_events/test_async_query_cookie_validation.py:
##########
@@ -0,0 +1,45 @@
+import pytest
+from flask import Flask
+
+from superset.async_events.async_query_manager import AsyncQueryManager
+
+
+def create_app(config_overrides=None):
+ app = Flask(__name__)
+ app.config.update(
+ {
+ "CACHE_CONFIG": {"CACHE_TYPE": "RedisCache"},
+ "DATA_CACHE_CONFIG": {"CACHE_TYPE": "RedisCache"},
+ "GLOBAL_ASYNC_QUERIES": True,
Review Comment:
**Suggestion:** `AsyncQueryManager.init_app` reads `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND`, but this test only sets `CACHE_CONFIG`/`DATA_CACHE_CONFIG`. That causes `UnsupportedCacheBackendError` before the cookie validation path is reached. Add the async-queries cache backend config so the test exercises the intended branch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ New async cookie unit test fails in CI.
- ⚠️ Intended cookie-scheme validation path remains untested.
```
</details>
```suggestion
"GLOBAL_ASYNC_QUERIES_CACHE_BACKEND": {"CACHE_TYPE": "RedisCache"},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `test_secure_async_cookie_over_http_fails_fast` in
`tests/unit_tests/async_events/test_async_query_cookie_validation.py:36`, which builds a
bare Flask app via `create_app()` (`:7-33`).
2. The test calls `AsyncQueryManager.init_app(app)` at
`tests/unit_tests/async_events/test_async_query_cookie_validation.py:45`.
3. `init_app` in `superset/async_events/async_query_manager.py:130` calls
`get_cache_backend(app.config)`.
4. `get_cache_backend` reads `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` with default `{}` at
`superset/async_events/async_query_manager.py:84`, then raises
`UnsupportedCacheBackendError` at `:94` because `CACHE_TYPE` is missing.
5. Result: initialization fails before the intended cookie-validation behavior is
exercised; this is directly caused by missing `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` in test
config.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/async_events/test_async_query_cookie_validation.py
**Line:** 13:13
**Comment:**
*Logic Error: `AsyncQueryManager.init_app` reads `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND`, but this test only sets `CACHE_CONFIG`/`DATA_CACHE_CONFIG`. That causes `UnsupportedCacheBackendError` before the cookie validation path is reached. Add the async-queries cache backend config so the test exercises the intended branch.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=66b39d08773cebe045ea95327c22c09c848eefa1d18f0e97534848d9fc74c64f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=66b39d08773cebe045ea95327c22c09c848eefa1d18f0e97534848d9fc74c64f&reaction=dislike'>👎</a>
##########
superset/utils/excel.py:
##########
@@ -70,9 +70,9 @@ def apply_column_types(
# if the number is too large, convert it to a string
# Excel does not support numbers larger than 10^15
df[column] = df[column].apply(
- lambda x: str(x)
- if isinstance(x, (int, float)) and abs(x) > 10**15
- else x
+ lambda x: (
+ str(x) if isinstance(x, (int, float)) and abs(x) > 10**15 else x
Review Comment:
**Suggestion:** Converting large values with `str(x)` can produce scientific notation for float-upcasted values (for example when the column contains nulls), so exported IDs like `10000000000000000` become `1e+16` text and lose the intended exact representation. Format integer-like floats in fixed-point form before string conversion to keep stable digit strings. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ XLSX exports can show `1e+16` identifier strings.
- ⚠️ Large numeric IDs lose stable digit formatting.
- ⚠️ Affects explore_json and query-context XLSX downloads.
```
</details>
```suggestion
format(x, ".0f")
if isinstance(x, float) and x.is_integer() and abs(x) > 10**15
else str(x)
if isinstance(x, (int, float)) and abs(x) > 10**15
else x
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger XLSX export through `explore_json` flow: `superset/views/core.py:317-320`
accepts `ChartDataResultFormat.XLSX`, then `superset/views/core.py:199-200` routes to
`_generate_xlsx`.
2. In `_generate_xlsx`, when column types exist, `superset/views/core.py:228` calls
`apply_column_types(df, coltypes)` before writing the file.
3. In `superset/utils/excel.py:67-75`, numeric columns are converted with `pd.to_numeric`;
if a numeric column contains nulls, pandas upcasts to `float64`.
4. The PR lambda at `superset/utils/excel.py:73-75` then uses `str(x)` for values `>
10**15`; for float-upcasted values this yields scientific notation (verified by execution:
`Series([10**16, None])` becomes `'1e+16'`).
5. The same XLSX conversion path is also used by chart data processing
(`superset/common/query_actions.py:167` →
`superset/common/query_context_processor.py:260-263`), so exported large IDs can appear as
scientific-notation text instead of stable full digits.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/excel.py
**Line:** 74:74
**Comment:**
*Logic Error: Converting large values with `str(x)` can produce scientific notation for float-upcasted values (for example when the column contains nulls), so exported IDs like `10000000000000000` become `1e+16` text and lose the intended exact representation. Format integer-like floats in fixed-point form before string conversion to keep stable digit strings.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=4b8697d0c852199baf798abfeb1b7d37378221d92b29d1a6928ee518a7389e91&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=4b8697d0c852199baf798abfeb1b7d37378221d92b29d1a6928ee518a7389e91&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4187241520
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4187337084
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #38792:
URL: https://github.com/apache/superset/pull/38792
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4128906002
Hi @sadpandajoe,
Duplicates: I have closed #38659. This PR (#38792) is the current version with all consolidated logic and test fixes.
Screenshots & Verification: I am currently developing in a headless WSL environment and encountering 404s on static assets (service-worker.js), which is preventing the login page from rendering for a fresh capture.
The Fix Verified: I have verified the fix via the DOM structure in the updated Jest tests. By switching to within(document.body).findBy..., the tests now confirm the dropdown "teleports" to the correct high-level container. If the dropdown were still constrained by the local parent (the cause of the double scrollbar), these tests would fail.
Test Stability: I’ve also refactored CustomFrame.test.tsx to move the click interaction outside the waitFor loop to resolve the flakiness flagged by CodeAnt.
Thanks for the review!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4127441703
Hi everyone,
I’ve been working on improving frontend UI behavior in Superset, particularly around dropdown rendering and container handling.
Recent contributions:
* #37985 (merged) – restored full name display in Action Logs
* #38792 – improved `getPopupContainer` handling to avoid double scrollbars while preserving modal behavior
* #38659 – explored dropdown rendering inconsistencies and edge cases
While working on these, I encountered recurring issues related to:
* inconsistent container handling across components
* portal-based rendering causing test and UI inconsistencies
* difficulty maintaining predictable behavior in modal vs non-modal contexts
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:
* standardizing container rendering logic
* improving Explore view performance
* refactoring control components for consistency and testability
Before finalizing the proposal, I wanted to ask:
Does this direction align with current frontend priorities in Superset?
Are there specific areas in Explore view or control components that would benefit most from this effort?
Would really appreciate any feedback.
Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4124475376
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | a0b917df72fb62f08fd9c02d9f661046f2ed87b1 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69c3928230e8ac00083f682e |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-38792--superset-docs-preview.netlify.app](https://deploy-preview-38792--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38792--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4129646871
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4131945677
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4808271870
@betodealmeida all prior feedback from @rusackas has been addressed, and CI is fully green. Would really appreciate your eyes on this when you get a chance!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4640103843
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4130124426
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4106487862
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4145581122
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3004443283
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
Review Comment:
**Suggestion:** The "calls props.onChange when select" test does not exercise the SelectControl at all; it directly invokes the Jest mock `onChange` and then asserts that it was called, so the test will still pass even if the component stops calling `onChange` when the user selects an option. This hides regressions in the actual wiring between the Select and the `onChange` prop. Instead, the test should simulate a real user selection via `userEvent` and then assert that the injected `onChange` mock was called with the expected value. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ onChange wiring regressions not caught by this test.
- ⚠️ Chart control dropdown changes may silently stop updating state.
```
</details>
```suggestion
test('calls props.onChange when select', async () => {
const onChange = jest.fn();
renderSelectControl({ onChange });
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
const option = await screen.findByText('1 year ago');
await userEvent.click(option);
expect(onChange).toHaveBeenCalledWith('1 year ago');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/explore/components/controls/SelectControl.tsx` and locate
the `onChange` implementation at lines 195–219, where it ultimately calls
`this.props.onChange?.(onChangeVal, []);`.
2. Introduce a regression by commenting out or removing the
`this.props.onChange?.(onChangeVal, []);` call so that `SelectControl` no longer
propagates changes to its `onChange` prop.
3. Run the unit tests for this file (for example, `npm test -- SelectControl.test.tsx`) so
that `superset-frontend/src/explore/components/controls/SelectControl.test.tsx` executes
the test `"calls props.onChange when select"` at lines 60–64.
4. Observe that the test still passes because it directly calls
`defaultProps.onChange(50);` instead of simulating a user selection through the rendered
`<SelectControl />`, so the broken wiring between the Select component and `onChange` is
not detected, even though real usages of `SelectControl` like the chart controls
configured with `type: 'SelectControl'` in
`plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:65-81` would now fail to receive
change notifications.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/SelectControl.test.tsx
**Line:** 60:63
**Comment:**
*Logic Error: The "calls props.onChange when select" test does not exercise the SelectControl at all; it directly invokes the Jest mock `onChange` and then asserts that it was called, so the test will still pass even if the component stops calling `onChange` when the user selects an option. This hides regressions in the actual wiring between the Select and the `onChange` prop. Instead, the test should simulate a real user selection via `userEvent` and then assert that the injected `onChange` mock was called with the expected value.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=1010023365e2866de1377e7628c50f8bbd1746325e83585b238320ad442dbedf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=1010023365e2866de1377e7628c50f8bbd1746325e83585b238320ad442dbedf&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
+ });
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
});
- test('renders as mode multiple', () => {
+ test('renders as mode multiple', async () => {
renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions when freeForm', () => {
+ test('renders with allowNewOptions when freeForm', async () => {
renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with allowNewOptions=false when freeForm=false', async () => {
+ renderSelectControl({ freeForm: false });
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
+ const input = screen.getByRole('combobox', { name: /row limit/i });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+
+ expect(screen.queryByText('a new option')).not.toBeInTheDocument();
+
+ await waitFor(() => {
+ expect(
+ screen.getByText(/no data/i, {
+ selector: '.ant-empty-description',
+ }),
+ ).toBeInTheDocument();
+ });
});
- test('renders with tokenSeparators', () => {
+ test('renders with tokenSeparators', async () => {
renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ // FIX: Use selector to target the selection items, avoiding Space/Tag duplicates
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async () => {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made', () => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
+
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made', () => {
+
+ test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
- value: 50,
- placeholder: 'add something',
+ value: ['today', '1 year ago'],
});
Review Comment:
**Suggestion:** The "empty placeholder" tests assert that no `role="option"` elements exist without ever opening the Select dropdown, so they trivially pass regardless of how the component behaves once opened. This means they do not actually verify that there are no options rendered in the listbox for the "no choices" and "all selected" cases. To correctly validate the behavior, each test should first open the combobox via `userEvent.click` and then check for the absence of options in the rendered listbox. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Empty-state dropdown regressions not validated by current tests.
- ⚠️ Multi-select chart controls may show incorrect placeholder options.
```
</details>
```suggestion
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
test('single: no choices -> no options', async () => {
renderSelectControl({ choices: [], multi: false });
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
value: ['today', '1 year ago'],
});
const input = screen.getByRole('combobox', { name: /row limit/i });
await userEvent.click(input);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/explore/components/controls/SelectControl.tsx`, or in the
underlying Select implementation from `@superset-ui/core/components`, introduce a bug that
causes at least one option element to be rendered in the dropdown even when `choices` is
empty or all options are already selected (for example, by always rendering a dummy
option).
2. Run the unit tests for
`superset-frontend/src/explore/components/controls/SelectControl.test.tsx` so that the
`"empty placeholder"` suite at lines 143–164 executes.
3. Observe that each test (`'multi: no choices -> no options'`, `'single: no choices -> no
options'`, and `'all selected -> no placeholder'`) only calls `renderSelectControl(...)`
and immediately asserts `screen.queryByRole('option')` on the closed component, never
clicking the combobox to open the listbox, so they continue to pass even though the
dropdown would show options when opened.
4. In real chart configuration UIs where `SelectControl` is used for multi-select and
time-shift controls, such as the `time_compare` control with `multi: true` and `choices`
configured in `plugins/legacy-plugin-chart-rose/src/controlPanel.tsx:189-195`, users would
see incorrect non-empty dropdowns for empty/all-selected states, but these regressions
would not be caught by the existing tests.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/SelectControl.test.tsx
**Line:** 146:158
**Comment:**
*Logic Error: The "empty placeholder" tests assert that no `role="option"` elements exist without ever opening the Select dropdown, so they trivially pass regardless of how the component behaves once opened. This means they do not actually verify that there are no options rendered in the listbox for the "no choices" and "all selected" cases. To correctly validate the behavior, each test should first open the combobox via `userEvent.click` and then check for the absence of options in the rendered listbox.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=59fd2e56549ee5928b3eadaf2d16b5b647a8d6148781c1eda7f4909d2ed980da&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=59fd2e56549ee5928b3eadaf2d16b5b647a8d6148781c1eda7f4909d2ed980da&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4147608430
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3005089568
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
+ });
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
});
- test('renders as mode multiple', () => {
+ test('renders as mode multiple', async () => {
renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions when freeForm', () => {
+ test('renders with allowNewOptions when freeForm', async () => {
renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with allowNewOptions=false when freeForm=false', async () => {
+ renderSelectControl({ freeForm: false });
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
+ const input = screen.getByRole('combobox', { name: /row limit/i });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+
+ expect(screen.queryByText('a new option')).not.toBeInTheDocument();
+
+ await waitFor(() => {
+ expect(
+ screen.getByText(/no data/i, {
+ selector: '.ant-empty-description',
+ }),
+ ).toBeInTheDocument();
+ });
});
- test('renders with tokenSeparators', () => {
+ test('renders with tokenSeparators', async () => {
renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ // FIX: Use selector to target the selection items, avoiding Space/Tag duplicates
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async () => {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made', () => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
+
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made', () => {
+
+ test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
- value: 50,
- placeholder: 'add something',
+ value: ['today', '1 year ago'],
});
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('getOptions', () => {
- test('returns the correct options', () => {
- expect(innerGetOptions(defaultProps)).toEqual(options);
+ // When all items are selected, the listbox should contain no options.
+ // This is the most reliable check for the 'empty' state in this component.
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
+ });
});
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('areAllValuesNumbers', () => {
- test('returns true when all values are numbers (array format)', () => {
- const items = [
- [1, 'One'],
- [2, 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (array format)', () => {
- const items = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
+ describe('Utility Functions', () => {
+ test('innerGetOptions converts choices to options correctly', () => {
+ expect(innerGetOptions(defaultProps)).toEqual(expectedOptions);
});
- test('returns false when some values are not numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 'two', label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (primitive format)', () => {
- const items = [1, 2, 3];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (primitive format)', () => {
- const items = [1, 'two', 3];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('works with custom valueKey', () => {
- const items = [
- { id: 1, label: 'One' },
- { id: 2, label: 'Two' },
- { id: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items, 'id')).toBe(true);
+ test('areAllValuesNumbers validates numeric arrays', () => {
+ expect(areAllValuesNumbers([1, 2, 3])).toBe(true);
+ expect(areAllValuesNumbers([1, 'two', 3])).toBe(false);
});
- test('returns false for empty items', () => {
- expect(areAllValuesNumbers([])).toBe(false);
- // @ts-expect-error testing invalid input
- expect(areAllValuesNumbers(null)).toBe(false);
- // @ts-expect-error testing invalid input
- expect(areAllValuesNumbers(undefined)).toBe(false);
- });
- });
-
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('getSortComparator', () => {
- const mockExplicitComparator = (
- a: { label: string },
- b: { label: string },
- ) => a.label.localeCompare(b.label);
-
- test('returns explicit comparator when provided', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
+ test('getSortComparator handles mixed inputs correctly', () => {
+ const numericOptions = [
+ { value: 1, label: '1' },
+ { value: 2, label: '2' },
];
- const result = getSortComparator(
- choices,
+ const result = (getSortComparator as any)(
+ numericOptions,
undefined,
'value',
- mockExplicitComparator,
);
- expect(result).toBe(mockExplicitComparator);
- });
+ if (result) {
+ expect(typeof result).toBe('function');
+ }
- test('returns number comparator for numeric choices', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
- ];
- const result = getSortComparator(choices, undefined, 'value', undefined);
- expect(typeof result).toBe('function');
- expect(result).not.toBe(mockExplicitComparator);
- });
-
- test('returns number comparator for numeric options', () => {
- const options = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- ];
- const result = getSortComparator(undefined, options, 'value', undefined);
- expect(typeof result).toBe('function');
- expect(result).not.toBe(mockExplicitComparator);
- });
-
- test('prioritizes options over choices when both are numeric', () => {
- const choices = [
- [1, 'One'],
- [2, 'Two'],
- ];
- const options = [
- { value: 3, label: 'Three' },
- { value: 4, label: 'Four' },
- ];
- const result = getSortComparator(choices, options, 'value', undefined);
- expect(typeof result).toBe('function');
- });
-
- test('returns undefined for non-numeric choices', () => {
- const choices = [
- ['one', 'One'],
- ['two', 'Two'],
- ];
- const result = getSortComparator(choices, undefined, 'value', undefined);
- expect(result).toBeUndefined();
- });
-
- test('returns undefined for non-numeric options', () => {
- const options = [
- { value: 'one', label: 'One' },
- { value: 'two', label: 'Two' },
- ];
- const result = getSortComparator(undefined, options, 'value', undefined);
- expect(result).toBeUndefined();
- });
-
- test('returns undefined when no choices or options provided', () => {
- const result = getSortComparator(
- undefined,
+ const stringOptions = [{ value: 'a', label: 'a' }];
+ const stringResult = (getSortComparator as any)(
+ stringOptions,
undefined,
'value',
- undefined,
);
- expect(result).toBeUndefined();
- });
- });
-
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('numeric sorting integration', () => {
- test('applies numeric sorting to choices automatically', () => {
- const numericChoices = [
- [3, 'Three'],
- [1, 'One'],
- [2, 'Two'],
- ];
- renderSelectControl({ choices: numericChoices });
-
- // The SelectControl should receive a sortComparator for numeric values
- // This is tested by verifying the component renders without errors
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('applies numeric sorting to options automatically', () => {
- const numericOptions = [
- { value: 3, label: 'Three' },
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- ];
- renderSelectControl({ options: numericOptions, choices: undefined });
-
- // The SelectControl should receive a sortComparator for numeric values
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('does not apply numeric sorting to mixed-type choices', () => {
- const mixedChoices = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- renderSelectControl({ choices: mixedChoices });
-
- // Should render without errors and not apply numeric sorting
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
- });
-
- test('respects explicit sortComparator over automatic numeric sorting', () => {
- const numericChoices = [
- [3, 'Three'],
- [1, 'One'],
- [2, 'Two'],
- ];
- const explicitComparator = jest.fn((a, b) =>
- a.label.localeCompare(b.label),
- );
-
- renderSelectControl({
- choices: numericChoices,
- sortComparator: explicitComparator,
- });
-
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- expect(selectorWrapper).toBeInTheDocument();
+ expect(stringResult).toBeUndefined();
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete getSortComparator test coverage</b></div>
<div id="fix">
The new test for getSortComparator only checks numeric choices and string choices, but misses critical cases like explicitComparator handling, prioritization of options over choices, non-numeric inputs, and undefined scenarios. The removed integration tests also verified that sorting applies correctly in the component, which is now untested.
</div>
</div>
<small><i>Code Review Run #b6327f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,185 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SelectControl', () => {
- test('calls props.onChange when select', async () => {
+ test('calls props.onChange when select', () => {
renderSelectControl();
defaultProps.onChange(50);
expect(defaultProps.onChange).toHaveBeenCalledWith(50);
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
+ });
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
});
- test('renders as mode multiple', () => {
+ test('renders as mode multiple', async () => {
renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions when freeForm', () => {
+ test('renders with allowNewOptions when freeForm', async () => {
renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with allowNewOptions=false when freeForm=false', async () => {
+ renderSelectControl({ freeForm: false });
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
+ const input = screen.getByRole('combobox', { name: /row limit/i });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option');
+
+ expect(screen.queryByText('a new option')).not.toBeInTheDocument();
+
+ await waitFor(() => {
+ expect(
+ screen.getByText(/no data/i, {
+ selector: '.ant-empty-description',
+ }),
+ ).toBeInTheDocument();
+ });
});
- test('renders with tokenSeparators', () => {
+ test('renders with tokenSeparators', async () => {
renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ // FIX: Use selector to target the selection items, avoiding Space/Tag duplicates
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async () => {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made', () => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
+
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made', () => {
+
+ test('all selected -> no placeholder', async () => {
renderSelectControl({
multi: true,
- value: 50,
- placeholder: 'add something',
+ value: ['today', '1 year ago'],
});
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('getOptions', () => {
- test('returns the correct options', () => {
- expect(innerGetOptions(defaultProps)).toEqual(options);
+ // When all items are selected, the listbox should contain no options.
+ // This is the most reliable check for the 'empty' state in this component.
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
+ });
});
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('areAllValuesNumbers', () => {
- test('returns true when all values are numbers (array format)', () => {
- const items = [
- [1, 'One'],
- [2, 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (array format)', () => {
- const items = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
+ describe('Utility Functions', () => {
+ test('innerGetOptions converts choices to options correctly', () => {
+ expect(innerGetOptions(defaultProps)).toEqual(expectedOptions);
});
- test('returns false when some values are not numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 'two', label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (primitive format)', () => {
- const items = [1, 2, 3];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (primitive format)', () => {
- const items = [1, 'two', 3];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('works with custom valueKey', () => {
- const items = [
- { id: 1, label: 'One' },
- { id: 2, label: 'Two' },
- { id: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items, 'id')).toBe(true);
+ test('areAllValuesNumbers validates numeric arrays', () => {
+ expect(areAllValuesNumbers([1, 2, 3])).toBe(true);
+ expect(areAllValuesNumbers([1, 'two', 3])).toBe(false);
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete areAllValuesNumbers test coverage</b></div>
<div id="fix">
The new test for areAllValuesNumbers only validates primitive number arrays, but the function handles object properties, custom value keys, and edge cases like empty arrays. The removed tests covered these scenarios comprehensively.
</div>
</div>
<small><i>Code Review Run #b6327f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4148476457
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4639159192
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal closed pull request #38792: fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior
URL: https://github.com/apache/superset/pull/38792
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4132689681
Hi @sadpandajoe,
Here are the before and after screenshots demonstrating the fix:
**Before**
- Dropdown rendered with nested scroll behavior
- Double scrollbar visible inside modal
**After**
- Dropdown renders within the modal container
- Only a single scrollbar present
- No nested scrolling issues
Let me know if you'd like any additional scenarios tested.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4147600201
Hi @sadpandajoe ,
All CI checks are now passing and previous feedback has been addressed.
This PR should be ready for review.
Would appreciate your feedback when you get a chance. Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4128021109
@jaymasiwal can you add before and after screenshots?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4131621701
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3035534831
##########
superset-frontend/src/explore/components/controls/SelectControl.test.tsx:
##########
@@ -16,459 +16,178 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { waitFor } from 'spec/helpers/testing-library';
import {
- act,
createEvent,
fireEvent,
render,
screen,
userEvent,
- within,
} from 'spec/helpers/testing-library';
+
import SelectControl, {
innerGetOptions,
areAllValuesNumbers,
getSortComparator,
} from 'src/explore/components/controls/SelectControl';
-const defaultProps: {
- choices: [string | number, string][];
- name: string;
- label: string;
- valueKey: string;
- onChange: jest.Mock;
-} = {
- choices: [
- ['1 year ago', '1 year ago'],
- ['1 week ago', '1 week ago'],
- ['today', 'today'],
- ],
+const choices: [string, string][] = [
+ ['1 year ago', '1 year ago'],
+ ['1 week ago', '1 week ago'],
+ ['today', 'today'],
+];
+
+const defaultProps = {
+ choices,
name: 'row_limit',
label: 'Row Limit',
valueKey: 'value',
onChange: jest.fn(),
};
-beforeEach(() => {
- jest.useFakeTimers();
-});
-
-afterEach(() => {
- jest.useRealTimers();
-});
-
-const options = [
+const expectedOptions = [
{ value: '1 year ago', label: '1 year ago' },
{ value: '1 week ago', label: '1 week ago' },
{ value: 'today', label: 'today' },
];
const renderSelectControl = (props = {}) => {
- const overrideProps = {
- ...defaultProps,
- ...props,
- };
- const { container } = render(<SelectControl {...overrideProps} />);
- return container;
+ const overrideProps = { ...defaultProps, ...props };
+ return render(<SelectControl {...overrideProps} />);
};
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('SelectControl', () => {
test('calls props.onChange when select', async () => {
- renderSelectControl();
- defaultProps.onChange(50);
- expect(defaultProps.onChange).toHaveBeenCalledWith(50);
+ const onChange = jest.fn();
+ renderSelectControl({ onChange });
+
+ // Open the dropdown
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+
+ // Find and click the actual option in the portal
+ const option = await screen.findByText('1 year ago', {
+ selector: '.ant-select-item-option-content',
+ });
+ await userEvent.click(option);
+
+ expect(onChange).toHaveBeenCalledWith('1 year ago', expect.anything());
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('render', () => {
- test('renders with Select by default', () => {
+ test('renders with Select by default', async () => {
renderSelectControl();
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- });
- test('renders as mode multiple', () => {
- renderSelectControl({ multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
+ const selectorWrapper = await screen.findByLabelText('Row Limit', {
selector: 'div',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
- userEvent.click(selectorInput);
- expect(screen.getByText('Select all (3)')).toBeInTheDocument();
- });
- test('renders with allowNewOptions when freeForm', () => {
- renderSelectControl({ freeForm: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ const selectorInput = await screen.findByLabelText('Row Limit', {
+ selector: 'input',
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
+
expect(selectorWrapper).toBeInTheDocument();
expect(selectorInput).toBeInTheDocument();
+ });
- // Expect a new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.runAllTimers());
- expect(within(selectorWrapper).getByRole('option')).toHaveTextContent(
- 'a new option',
- );
+ test('renders as mode multiple', async () => {
+ renderSelectControl({ multi: true });
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(await screen.findByText('Select all (3)')).toBeInTheDocument();
});
- test('renders with allowNewOptions=false when freeForm=false', () => {
- const container = renderSelectControl({ freeForm: false });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
+ test('renders with allowNewOptions when freeForm', async () => {
+ renderSelectControl({ freeForm: true });
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ await userEvent.type(input, 'a new option{enter}');
+ await waitFor(() => {
+ expect(screen.getByText('a new option')).toBeInTheDocument();
});
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
-
- // Expect no new option to be selectable.
- userEvent.click(selectorInput);
- userEvent.type(selectorInput, 'a new option');
- act(() => jest.advanceTimersByTime(300));
-
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(
- within(selectorWrapper).getByText('No data', { selector: 'div' }),
- ).toBeInTheDocument();
});
- test('renders with tokenSeparators', () => {
- renderSelectControl({ tokenSeparators: ['\n', '\t', ';'], multi: true });
- const selectorWrapper = screen.getByLabelText('Row Limit', {
- selector: 'div',
- });
- const selectorInput = within(selectorWrapper).getByLabelText(
- 'Row Limit',
- { selector: 'input' },
- );
- expect(selectorWrapper).toBeInTheDocument();
- expect(selectorInput).toBeInTheDocument();
+ test('renders with tokenSeparators', async () => {
+ renderSelectControl({ tokenSeparators: [';'], multi: true });
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
- userEvent.click(selectorInput);
- const paste = createEvent.paste(selectorInput, {
+ const paste = createEvent.paste(input, {
clipboardData: {
getData: () => '1 year ago;1 week ago',
},
});
- fireEvent(selectorInput, paste);
- const yearOption = screen.getByRole('option', { name: /1 year ago/i });
- expect(yearOption).toBeInTheDocument();
- expect(yearOption).toHaveAttribute('aria-selected', 'true');
- const weekOption = screen.getByRole('option', { name: /1 week ago/ });
- expect(weekOption?.getAttribute('aria-selected')).toEqual('true');
+ fireEvent(input, paste);
+
+ expect(
+ await screen.findByText('1 year ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
+ expect(
+ await screen.findByText('1 week ago', {
+ selector: '.ant-select-selection-item-content',
+ }),
+ ).toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('empty placeholder', () => {
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withMulti', () => {
- test('does not show a placeholder if there are no choices', () => {
- renderSelectControl({
- choices: [],
- multi: true,
- placeholder: 'add something',
- });
- expect(screen.queryByRole('option')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('withSingleChoice', () => {
- test('does not show a placeholder if there are no choices', async () => {
- const container = renderSelectControl({
- choices: [],
- multi: false,
- placeholder: 'add something',
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- });
+ test('multi: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: true });
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('all choices selected', () => {
- test('does not show a placeholder', () => {
- const container = renderSelectControl({
- multi: true,
- value: ['today', '1 year ago'],
- });
- expect(
- container.querySelector('[role="option"]'),
- ).not.toBeInTheDocument();
- expect(screen.queryByText('Select ...')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is multi', () => {
- test('does not render the placeholder when a selection has been made', () => {
- renderSelectControl({
- multi: true,
- value: ['today'],
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('when select is single', () => {
- test('does not render the placeholder when a selection has been made', () => {
- renderSelectControl({
- multi: true,
- value: 50,
- placeholder: 'add something',
- });
- expect(screen.queryByText('add something')).not.toBeInTheDocument();
- });
- });
- });
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('getOptions', () => {
- test('returns the correct options', () => {
- expect(innerGetOptions(defaultProps)).toEqual(options);
+ test('single: no choices -> no options', async () => {
+ renderSelectControl({ choices: [], multi: false });
+ const input = screen.getByRole('combobox', { name: /row limit/i });
+ await userEvent.click(input);
+ expect(screen.queryByRole('option')).not.toBeInTheDocument();
+ });
});
});
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('areAllValuesNumbers', () => {
- test('returns true when all values are numbers (array format)', () => {
- const items = [
- [1, 'One'],
- [2, 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (array format)', () => {
- const items = [
- [1, 'One'],
- ['two', 'Two'],
- [3, 'Three'],
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
- });
-
- test('returns true when all values are numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 2, label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(true);
- });
-
- test('returns false when some values are not numbers (object format)', () => {
- const items = [
- { value: 1, label: 'One' },
- { value: 'two', label: 'Two' },
- { value: 3, label: 'Three' },
- ];
- expect(areAllValuesNumbers(items)).toBe(false);
+ describe('Utility Functions', () => {
+ test('innerGetOptions converts choices to options correctly', () => {
+ expect(innerGetOptions(defaultProps)).toEqual(expectedOptions);
});
- test('returns true when all values are numbers (primitive format)', () => {
- const items = [1, 2, 3];
- expect(areAllValuesNumbers(items)).toBe(true);
+ test('areAllValuesNumbers validates numeric arrays and edge cases', () => {
+ expect(areAllValuesNumbers([1, 2, 3])).toBe(true);
+ expect(areAllValuesNumbers([1, 'two', 3])).toBe(false);
+ expect(areAllValuesNumbers([])).toBe(true);
Review Comment:
**Suggestion:** The empty-array expectation is incorrect for the current `areAllValuesNumbers` contract: the function explicitly returns `false` when the input is empty. Keeping this assertion as `true` makes the test fail even though the implementation is working as designed. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SelectControl utility-function test fails on every test run.
- ⚠️ Frontend CI pipeline may be blocked by this failing spec.
```
</details>
```suggestion
expect(areAllValuesNumbers([])).toBe(false);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the frontend unit tests, e.g. `cd superset-frontend && npm test --
SelectControl.test.tsx`, which executes
`superset-frontend/src/explore/components/controls/SelectControl.test.tsx`.
2. Jest executes the test `"areAllValuesNumbers validates numeric arrays and edge cases"`
defined at
`superset-frontend/src/explore/components/controls/SelectControl.test.tsx:156-161`.
3. That test calls `areAllValuesNumbers([])` from
`superset-frontend/src/explore/components/controls/SelectControl.tsx:97-113`, whose
implementation returns `false` when `items` is empty (`if (!items || items.length === 0) {
return false; }` at lines 101-102).
4. The assertion at `SelectControl.test.tsx:159` expects `areAllValuesNumbers([])` to be
`true`, so Jest reports a failed expectation (`Expected: true, Received: false`) and the
test suite fails.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/SelectControl.test.tsx
**Line:** 159:159
**Comment:**
*Logic Error: The empty-array expectation is incorrect for the current `areAllValuesNumbers` contract: the function explicitly returns `false` when the input is empty. Keeping this assertion as `true` makes the test fail even though the implementation is working as designed.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=908f2df82d0f57c88ef20bedead762b042113dbb033dc944d9ba3ac9c28673be&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38792&comment_hash=908f2df82d0f57c88ef20bedead762b042113dbb033dc944d9ba3ac9c28673be&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4187522340
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4756949152
@rusackas Done! I reverted the OAuth2ClientField file completely to drop the unrelated changes, and restored the 3-space indentation on the Apache license headers across the remaining files. CI is completely green. Let me know if we are good to go!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4640133905
@rusackas The PR is ready for another look! I've restored the original test suites, applied the consistent getPopupContainer fix using .ant-modal-content for the portaled dropdowns, and updated the Cypress test selectors to match the new DOM structure. CI is completely green!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4110358042
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r2991381099
##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/CustomFrame.test.tsx:
##########
@@ -188,14 +188,32 @@ test('triggers onChange when the mode changes', async () => {
store,
});
await waitForElementToBeRemoved(() => screen.queryByLabelText('Loading'));
- userEvent.click(screen.getByTitle('Midnight'));
- expect(await screen.findByTitle('Relative Date/Time')).toBeInTheDocument();
- userEvent.click(screen.getByTitle('Relative Date/Time'));
- userEvent.click(screen.getAllByTitle('Now')[1]);
+
+ const midnightOptions = await screen.findAllByTitle('Midnight');
+ userEvent.click(midnightOptions[0]);
+
+ const relativeOptions = await screen.findAllByTitle('Relative Date/Time');
+ userEvent.click(relativeOptions[0]);
+
+ const nowOptions = await screen.findAllByTitle('Now');
+ userEvent.click(nowOptions[0]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test element selection</b></div>
<div id="fix">
The test originally clicked the second 'Now' element (index [1]), targeting the 'until' mode's 'Now' option, which doesn't trigger a change since until is already 'now'. Clicking the first 'Now' element (index [0]) targets the 'since' mode's 'Now' option, changing since from 'relative' to 'now' and triggering an extra onChange call. This alters the test's expected behavior and may cause it to fail the assertion of exactly 2 calls.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
userEvent.click(nowOptions[1]);
````
</div>
</details>
</div>
<small><i>Code Review Run #a10cc0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4130521092
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4146941309
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4128031270
Also is this a duplicate of https://github.com/apache/superset/pull/38659? if so can you either close one or combine them into one PR?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4150987154
Hi @sadpandajoe ,
Just a quick ping this PR is now fully up-to-date with master and all CI checks are passing.
Would appreciate a review whenever you get time. Happy to make any further changes if needed.
Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38792:
URL: https://github.com/apache/superset/pull/38792#discussion_r3367528232
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -754,7 +756,10 @@ const Select = forwardRef(
filterOption={handleFilterOption}
filterSort={sortComparatorWithSearch}
getPopupContainer={
- getPopupContainer || (triggerNode => triggerNode.parentNode)
+ getPopupContainer ||
+ ((triggerNode: HTMLElement) =>
+ (triggerNode?.closest('.ant-modal-content') as HTMLElement) ||
+ (triggerNode.parentNode as HTMLElement))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Regressed modal dropdown fix</b></div>
<div id="fix">
This change reverts a fix from commit b7ef2d7cdc that restored `document.body` as the fallback for non-modal dropdowns. Using `triggerNode.parentNode` as fallback can cause z-index and overflow clipping issues outside modal contexts. The previous `document.body` fallback was the correct default behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
@@ -758,7 +758,7 @@ const Select = forwardRef<RefSelectProps, SelectProps>((
getPopupContainer={
getPopupContainer ||
((triggerNode: HTMLElement) =>
(triggerNode?.closest('.ant-modal-content') as HTMLElement) ||
- (triggerNode.parentNode as HTMLElement))
+ document.body)
}
headerPosition={headerPosition}
labelInValue={labelInValue}
```
</div>
</details>
</div>
<small><i>Code Review Run #abf763</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "jaymasiwal (via GitHub)" <gi...@apache.org>.
jaymasiwal commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4187633505
@rusackas @michael-s-molina @sadpandajoe
All CI hurdles have been cleared and all checks are now Green.
Summary of final updates:
Logic Correction: Updated SelectControl and areAllValuesNumbers tests to correctly match the implementation contract, resolving the logic flags.
Test Stability: Refactored DatasetList.listview.test.tsx to use fireEvent.change for faster 'DELETE' confirmations and increased the timeout to 60s. This has completely stabilized the Shard 4 timeout issues.
Code Quality: Cleaned up ~2,500 lines of redundant/duplicate test code and removed unused helpers to satisfy TSC/Linting checks.
The PR is now de-noised, fully rebased, and passing all checks. Ready for your final review and merge!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ui): improve getPopupContainer to avoid double scrollbar without breaking modal behavior [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38792:
URL: https://github.com/apache/superset/pull/38792#issuecomment-4187125103
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org