bito-code-review[bot] commented on code in PR #40169: URL: https://github.com/apache/superset/pull/40169#discussion_r3248360628
########## superset-frontend/src/components/ListView/Filters/CompactFilterTrigger.test.tsx: ########## @@ -0,0 +1,122 @@ +/** + * 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'; + +const defaultProps = { + label: 'Owner', + hasValue: false, + onClear: jest.fn(), + children: <div data-testid="filter-content">Filter content</div>, +}; + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('renders the label', () => { + render(<CompactFilterTrigger {...defaultProps} />); + expect(screen.getByText('Owner')).toBeInTheDocument(); +}); + +test('renders as inactive pill with down chevron when hasValue is false', () => { + render(<CompactFilterTrigger {...defaultProps} />); + const pill = screen.getByTestId('compact-filter-pill'); + expect(pill).toBeInTheDocument(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'renders as inactive pill with down chevron when hasValue is false' only checks that the pill exists, but does not verify the down chevron is rendered or that the close icon is absent. To assert the behavior as claimed, add `expect(screen.queryByRole('button', { name: /close/i })).not.toBeInTheDocument();` to confirm the inactive state logic. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion test('renders as inactive pill with down chevron when hasValue is false', () => { render(<CompactFilterTrigger {...defaultProps} />); const pill = screen.getByTestId('compact-filter-pill'); expect(pill).toBeInTheDocument(); expect(screen.queryByRole('button', { name: /close/i })).not.toBeInTheDocument(); }); ```` </div> </details> </div> <small><i>Code Review Run #e8d33e</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/index.tsx: ########## @@ -145,30 +151,71 @@ function UIFilters( ); } if (input === 'datetime_range') { + const hasDateValue = + Array.isArray(initialValue) && initialValue.some(Boolean); + const dateTooltip = hasDateValue + ? (initialValue as (string | number)[]) + .filter(Boolean) + .join(' – ') + : undefined; return ( - <DateRangeFilter - ref={filterRefs[index]} - Header={Header} - initialValue={initialValue} + <CompactFilterTrigger key={key} - name={id} - onSubmit={value => updateFilterValue(index, value)} - dateFilterValueType={dateFilterValueType || 'unix'} - /> + label={Header} + hasValue={hasDateValue} + tooltipTitle={dateTooltip} + popupType="dialog" + onClear={() => { + filterRefs[index]?.current?.clearFilter?.(); + updateFilterValue(index, undefined); + }} + > + <FilterPopoverContent> + <DateRangeFilter + ref={filterRefs[index]} + Header={Header} + initialValue={initialValue} + name={id} + onSubmit={value => updateFilterValue(index, value)} + dateFilterValueType={dateFilterValueType || 'unix'} + /> + </FilterPopoverContent> + </CompactFilterTrigger> ); } if (input === 'numerical_range') { + const hasRangeValue = + Array.isArray(initialValue) && + initialValue.some(v => v !== null && v !== undefined); + const rangeTooltip = hasRangeValue + ? (initialValue as (number | null | undefined)[]) + .filter(v => v !== null && v !== undefined) + .join(' – ') + : undefined; return ( - <NumericalRangeFilter - ref={filterRefs[index]} - Header={Header} - initialValue={initialValue} - min={min} - max={max} + <CompactFilterTrigger key={key} - name={id} - onSubmit={value => updateFilterValue(index, value)} - /> + label={Header} + hasValue={hasRangeValue} + tooltipTitle={rangeTooltip} + popupType="dialog" + onClear={() => { + filterRefs[index]?.current?.clearFilter?.(); + updateFilterValue(index, undefined); + }} + > Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Conflicting filter update in onClear</b></div> <div id="fix"> The `onClear` callback calls `updateFilterValue` with `undefined` after invoking `clearFilter`, but `clearFilter` already updates the filter value via `onSubmit([])`. This sets the value to `[]` then `undefined`, causing conflicting state updates. Remove the `updateFilterValue` call to use the correct cleared state from `clearFilter`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion onClear={() => { filterRefs[index]?.current?.clearFilter?.(); }} ```` </div> </details> </div> <small><i>Code Review Run #e8d33e</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/index.tsx: ########## @@ -145,30 +151,71 @@ function UIFilters( ); } if (input === 'datetime_range') { + const hasDateValue = + Array.isArray(initialValue) && initialValue.some(Boolean); + const dateTooltip = hasDateValue + ? (initialValue as (string | number)[]) + .filter(Boolean) + .join(' – ') + : undefined; return ( - <DateRangeFilter - ref={filterRefs[index]} - Header={Header} - initialValue={initialValue} + <CompactFilterTrigger key={key} - name={id} - onSubmit={value => updateFilterValue(index, value)} - dateFilterValueType={dateFilterValueType || 'unix'} - /> + label={Header} + hasValue={hasDateValue} + tooltipTitle={dateTooltip} + popupType="dialog" + onClear={() => { + filterRefs[index]?.current?.clearFilter?.(); + updateFilterValue(index, undefined); + }} + > + <FilterPopoverContent> + <DateRangeFilter + ref={filterRefs[index]} + Header={Header} + initialValue={initialValue} + name={id} + onSubmit={value => updateFilterValue(index, value)} + dateFilterValueType={dateFilterValueType || 'unix'} + /> + </FilterPopoverContent> + </CompactFilterTrigger> ); } if (input === 'numerical_range') { + const hasRangeValue = + Array.isArray(initialValue) && + initialValue.some(v => v !== null && v !== undefined); + const rangeTooltip = hasRangeValue + ? (initialValue as (number | null | undefined)[]) + .filter(v => v !== null && v !== undefined) + .join(' – ') + : undefined; return ( - <NumericalRangeFilter - ref={filterRefs[index]} - Header={Header} - initialValue={initialValue} - min={min} - max={max} + <CompactFilterTrigger key={key} - name={id} - onSubmit={value => updateFilterValue(index, value)} - /> + label={Header} + hasValue={hasRangeValue} + tooltipTitle={rangeTooltip} + popupType="dialog" + onClear={() => { + filterRefs[index]?.current?.clearFilter?.(); + updateFilterValue(index, undefined); + }} + > Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Conflicting filter update in onClear</b></div> <div id="fix"> The `onClear` callback calls `updateFilterValue` with `undefined` after invoking `clearFilter`, but `clearFilter` already updates the filter value via `onSubmit([null, null])`. This sets the value to `[null, null]` then `undefined`, causing conflicting state updates. Remove the `updateFilterValue` call to use the correct cleared state from `clearFilter`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion onClear={() => { filterRefs[index]?.current?.clearFilter?.(); }} ```` </div> </details> </div> <small><i>Code Review Run #e8d33e</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]
