codeant-ai-for-open-source[bot] commented on code in PR #37017:
URL: https://github.com/apache/superset/pull/37017#discussion_r2677151083
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -602,6 +602,81 @@ describe('SelectFilterPlugin', () => {
expect(options[2]).toHaveTextContent('alpha');
});
+ test('Select boolean FALSE value in single-select mode', async () => {
+ const setDataMaskMock = jest.fn();
+ render(
+ // @ts-ignore
+ <SelectFilterPlugin
+ // @ts-ignore
+ {...transformProps({
+ ...selectMultipleProps,
+ formData: {
+ ...selectMultipleProps.formData,
+ multiSelect: false,
+ groupby: ['is_active'],
+ },
+ queriesData: [
+ {
+ rowcount: 2,
+ colnames: ['is_active'],
+ coltypes: [1],
+ data: [{ is_active: true }, { is_active: false }],
+ applied_filters: [{ column: 'is_active' }],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: undefined },
+ })}
+ setDataMask={setDataMaskMock}
+ showOverflow={false}
+ />,
+ {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: {
+ 'test-filter': {
+ name: 'Test Filter',
+ },
+ },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: {
+ value: undefined,
+ },
+ },
+ },
+ },
+ },
+ );
+
+ const filterSelect = screen.getAllByRole('combobox')[0];
+ userEvent.click(filterSelect);
+
+ const falseOption = await screen.findByRole('option', { name: /false/i });
+ expect(falseOption).toBeInTheDocument();
+ userEvent.click(falseOption);
Review Comment:
**Suggestion:** Missing await on the user interaction:
`userEvent.click(falseOption)` is asynchronous in modern user-event versions
and can cause a race where the assertion runs before the component's update and
the mocked `setDataMask` call happen; await the click to ensure the click
completes before asserting. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await userEvent.click(falseOption);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Modern versions of @testing-library/user-event expose async behaviour for
certain interactions.
Awaiting the click removes a potential race between the click handling and
the assertion about the
mocked setDataMask being invoked. The improved code directly addresses a
plausible flake in this hunk
with a single, minimal change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
**Line:** 660:660
**Comment:**
*Race Condition: Missing await on the user interaction:
`userEvent.click(falseOption)` is asynchronous in modern user-event versions
and can cause a race where the assertion runs before the component's update and
the mocked `setDataMask` call happen; await the click to ensure the click
completes before asserting.
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>
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -514,7 +514,7 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
allowClear
allowNewOptions={!searchAllOptions && creatable !== false}
allowSelectAll={!searchAllOptions}
- value={filterState.value || []}
+ value={multiSelect ? filterState.value || [] : filterState.value}
Review Comment:
**Suggestion:** When `multiSelect` is true the Select component expects an
array; if `filterState.value` is a single scalar (e.g. a string like 'false')
it will be passed incorrectly — coerce non-array values into a single-element
array so multiple mode always receives an array. [type error]
**Severity Level:** Minor ⚠️
```suggestion
value={
multiSelect
? Array.isArray(filterState.value)
? filterState.value
: filterState.value != null
? [filterState.value]
: []
: filterState.value
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a practical fix: when filterState.value arrives as a scalar (e.g.,
from persisted state or another codepath), the Select in multiple mode expects
an array. Coercing non-array values into a single-element array prevents
runtime/behavioral bugs. The improved code is correct and addresses a plausible
mismatch.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
**Line:** 517:517
**Comment:**
*Type Error: When `multiSelect` is true the Select component expects an
array; if `filterState.value` is a single scalar (e.g. a string like 'false')
it will be passed incorrectly — coerce non-array values into a single-element
array so multiple mode always receives an array.
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>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
