bito-code-review[bot] commented on code in PR #40169:
URL: https://github.com/apache/superset/pull/40169#discussion_r3326349991
##########
superset-frontend/src/components/ListView/ListView.test.tsx:
##########
@@ -341,7 +345,10 @@ describe('ListView', () => {
initialSort: [{ id: 'something' }],
});
- const sortSelect = screen.getByTestId('card-sort-select');
+ const sortSelectContainer = screen.getByTestId('card-sort-select');
+ const sortSelect = sortSelectContainer.querySelector(
+ '[data-test="compact-filter-pill"]',
+ ) as HTMLElement;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent Testing Library pattern</b></div>
<div id="fix">
The new code uses `querySelector` to find `compact-filter-pill` inside
`card-sort-select`, bypassing Testing Library's async utilities and error
messages. The same file already uses the idiomatic `within()` pattern at line
286-288 for the same use case. This inconsistency reduces test debuggability
when the element is not found. Use
`within(sortSelectContainer).getByTestId('compact-filter-pill')` instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/components/ListView/ListView.test.tsx
+++ b/superset-frontend/src/components/ListView/ListView.test.tsx
@@ -345,8 +345,7 @@ describe('ListView', () => {
});
const sortSelectContainer = screen.getByTestId('card-sort-select');
- const sortSelect = sortSelectContainer.querySelector(
- '[data-test="compact-filter-pill"]',
- ) as HTMLElement;
+ const sortSelect =
within(sortSelectContainer).getByTestId('compact-filter-pill');
await userEvent.click(sortSelect);
```
</div>
</details>
</div>
<small><i>Code Review Run #1ee44a</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/components/ListView/Filters/CompactFilterTrigger.test.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import CompactFilterTrigger from './CompactFilterTrigger';
+
+// Base props without children — pass children as JSX to avoid
no-children-prop lint rule.
+const baseProps = {
+ label: 'Owner',
+ hasValue: false,
+ onClear: jest.fn(),
+};
+
+const defaultChildren = jest.fn(() => (
+ <div data-testid="filter-content">Filter content</div>
+));
+
+function renderTrigger(
+ props: Partial<
+ typeof baseProps & {
+ hasValue: boolean;
+ tooltipTitle?: string;
+ popupType?: 'listbox' | 'dialog';
+ }
+ > = {},
+ children = defaultChildren,
+) {
+ return render(
+ <CompactFilterTrigger {...baseProps} {...props}>
+ {children}
+ </CompactFilterTrigger>,
+ );
+}
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});
+
+test('renders the label', () => {
+ renderTrigger();
+ expect(screen.getByText('Owner')).toBeInTheDocument();
+});
+
+test('renders as inactive pill with down chevron when hasValue is false', ()
=> {
+ renderTrigger();
+ const pill = screen.getByTestId('compact-filter-pill');
+ expect(pill).toBeInTheDocument();
+ // No clear button when inactive
+ expect(screen.queryByTestId('compact-filter-clear')).not.toBeInTheDocument();
+});
+
+test('renders active state with clear icon when hasValue is true', () => {
+ renderTrigger({ hasValue: true });
+ expect(screen.getByTestId('compact-filter-clear')).toBeInTheDocument();
+});
+
+test('clear icon has descriptive aria-label matching the filter name', () => {
+ renderTrigger({ hasValue: true });
+ const clearIcon = screen.getByTestId('compact-filter-clear');
+ expect(clearIcon).toHaveAttribute('aria-label', 'Clear Owner filter');
+});
+
+test('clear icon is rendered inside the pill button', () => {
+ renderTrigger({ hasValue: true });
+ const pill = screen.getByTestId('compact-filter-pill');
+ const clearIcon = screen.getByTestId('compact-filter-clear');
+ expect(pill).toContainElement(clearIcon);
+});
+
+test('toggles aria-expanded when pill is clicked', async () => {
+ renderTrigger();
+ const pill = screen.getByTestId('compact-filter-pill');
+ expect(pill).toHaveAttribute('aria-expanded', 'false');
+ await userEvent.click(pill);
+ expect(pill).toHaveAttribute('aria-expanded', 'true');
+});
+
+test('calls onClear when clear icon is clicked', async () => {
+ const onClear = jest.fn();
+ renderTrigger({ hasValue: true, onClear } as any);
+ const clearIcon = screen.getByTestId('compact-filter-clear');
+ await userEvent.click(clearIcon);
+ expect(onClear).toHaveBeenCalledTimes(1);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing keyboard-accessible clear action</b></div>
<div id="fix">
The clear icon uses `Icons.CloseOutlined` (an icon component, not a button).
The test at line 97 clicks it via `data-test="compact-filter-clear"`, but
without `role="button"` or a keyboard-accessible wrapper, keyboard-only users
cannot trigger the clear action. The `FilterPill` button at line 163 handles
the dropdown toggle, but the clear icon needs its own interactive affordance.
</div>
</div>
<small><i>Code Review Run #1ee44a</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: [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]
