codeant-ai-for-open-source[bot] commented on code in PR #36209: URL: https://github.com/apache/superset/pull/36209#discussion_r2610986646
########## superset-frontend/src/components/ListView/Filters/index.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ListViewFilterOperator } from '../types'; +import UIFilters from './index'; + +const mockUpdateFilterValue = jest.fn(); + +beforeEach(() => { + mockUpdateFilterValue.mockClear(); +}); + +test('search filter uses id as input name when inputName is not provided', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('name'); +}); + +test('search filter uses inputName when provided instead of id', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + inputName: 'custom_search_name', + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('custom_search_name'); +}); + +test('search filter passes autoComplete prop correctly', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + autoComplete: 'new-password', + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.autocomplete).toBe('new-password'); Review Comment: **Suggestion:** This test assumes that an `autoComplete` field on the filter configuration is wired through `UIFilters` into the underlying input's `autocomplete` attribute, but that prop is not currently propagated, so asserting a specific `autocomplete` value will make the test fail even though the component behaves as implemented. [logic error] **Severity Level:** Minor ⚠️ ```suggestion test('search filter renders even when an unsupported autoComplete prop is provided', () => { const filters = [ { Header: 'Name', key: 'name', id: 'name', input: 'search' as const, operator: ListViewFilterOperator.Contains, autoComplete: 'new-password', }, ]; render( <UIFilters filters={filters} internalFilters={[]} updateFilterValue={mockUpdateFilterValue} />, ); const input = screen.getByTestId('filters-search') as HTMLInputElement; expect(input).toBeInTheDocument(); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> There is no propagation of an autoComplete prop from the filter configuration through UIFilters into SearchFilter: UIFilters does not read autoComplete and SearchFilter's <Input> does not receive an autoComplete prop. Asserting that autocomplete equals 'new-password' will fail against current implementation. Changing the test to verify rendering (or wiring the prop in the component if desired) is a real required fix for the test. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/ListView/Filters/index.test.tsx **Line:** 76:97 **Comment:** *Logic Error: This test assumes that an `autoComplete` field on the filter configuration is wired through `UIFilters` into the underlying input's `autocomplete` attribute, but that prop is not currently propagated, so asserting a specific `autocomplete` value will make the test fail even though the component behaves as implemented. 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/components/ListView/Filters/index.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ListViewFilterOperator } from '../types'; +import UIFilters from './index'; + +const mockUpdateFilterValue = jest.fn(); + +beforeEach(() => { + mockUpdateFilterValue.mockClear(); +}); + +test('search filter uses id as input name when inputName is not provided', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('name'); +}); + +test('search filter uses inputName when provided instead of id', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + inputName: 'custom_search_name', + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('custom_search_name'); +}); + +test('search filter passes autoComplete prop correctly', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + autoComplete: 'new-password', + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.autocomplete).toBe('new-password'); +}); + +test('renders multiple search filters with different inputName values', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + inputName: 'filter_name_search', + }, + { + Header: 'Description', + key: 'description', + id: 'description', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + // No inputName - should use id + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const inputs = screen.getAllByTestId('filters-search') as HTMLInputElement[]; + expect(inputs).toHaveLength(2); + expect(inputs[0].name).toBe('filter_name_search'); Review Comment: **Suggestion:** This test asserts that `inputName` changes the `name` of the first search input while the implementation always uses the filter `id` for the `name`, so the expectation on `inputs[0].name` will never match actual behavior and should be updated to reflect that both inputs use their `id` values. [logic error] **Severity Level:** Minor ⚠️ ```suggestion test('renders multiple search filters and uses id for input name', () => { const filters = [ { Header: 'Name', key: 'name', id: 'name', input: 'search' as const, operator: ListViewFilterOperator.Contains, inputName: 'filter_name_search', }, { Header: 'Description', key: 'description', id: 'description', input: 'search' as const, operator: ListViewFilterOperator.Contains, // No inputName - should use id }, ]; render( <UIFilters filters={filters} internalFilters={[]} updateFilterValue={mockUpdateFilterValue} />, ); const inputs = screen.getAllByTestId('filters-search') as HTMLInputElement[]; expect(inputs).toHaveLength(2); expect(inputs[0].name).toBe('name'); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The code shows UIFilters supplies the SearchFilter a name prop equal to id (name={id}) and SearchFilter uses that name directly on the input. The test expecting inputName to override the name is incorrect. Updating the test to expect the id-derived name (or implementing support for inputName) resolves a genuine mismatch between test expectations and component behavior. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/ListView/Filters/index.test.tsx **Line:** 100:130 **Comment:** *Logic Error: This test asserts that `inputName` changes the `name` of the first search input while the implementation always uses the filter `id` for the `name`, so the expectation on `inputs[0].name` will never match actual behavior and should be updated to reflect that both inputs use their `id` values. 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/pages/AnnotationLayerList/index.tsx: ########## @@ -255,6 +255,7 @@ function AnnotationLayersList({ id: 'name', input: 'search', operator: FilterOperator.Contains, + inputName: 'annotation_layer_list_search', }, Review Comment: **Suggestion:** The newly added `inputName` property on the filter config is never read by the ListView filter rendering logic (UIFilters only destructures and forwards a fixed set of properties and does not pass `inputName` to `SearchFilter`), so this setting has no effect and the intended accessibility change (e.g. attaching a specific attribute or name to the search input) will not occur; either the property needs to be wired through the filter layer, or removed to avoid a misleading, unused configuration. [logic error] **Severity Level:** Minor ⚠️ ```suggestion ``` <details> <summary><b>Why it matters? ⭐ </b></summary> I searched the repo for uses of "inputName" and the specific string "annotation_layer_list_search" and found no matches, so the new inputName property added in this PR is not being consumed anywhere. That means the intended accessibility/identification change won't take effect as-is and the prop is misleading. This is a valid functional concern (not just style): either the filter rendering layer needs to forward this prop into the search input or the property should be removed to avoid dead/unused config. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/AnnotationLayerList/index.tsx **Line:** 259:259 **Comment:** *Logic Error: The newly added `inputName` property on the filter config is never read by the ListView filter rendering logic (UIFilters only destructures and forwards a fixed set of properties and does not pass `inputName` to `SearchFilter`), so this setting has no effect and the intended accessibility change (e.g. attaching a specific attribute or name to the search input) will not occur; either the property needs to be wired through the filter layer, or removed to avoid a misleading, unused configuration. 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/pages/RowLevelSecurityList/index.tsx: ########## @@ -266,6 +266,7 @@ function RowLevelSecurityList(props: RLSProps) { id: 'name', input: 'search', operator: FilterOperator.StartsWith, + inputName: 'rls_list_search', Review Comment: **Suggestion:** The added `inputName` property on the filter config is not part of the `ListViewFilter` interface (as defined in `src/components/ListView/types.ts`), so TypeScript's excess property checks will reject this object literal and prevent this file from compiling; since nothing in the ListView/Filters/Search pipeline consumes `inputName`, the safest fix in this file is to remove the property. [type error] **Severity Level:** Minor ⚠️ ```suggestion ``` <details> <summary><b>Why it matters? ⭐ </b></summary> I checked the ListView filter type at src/components/ListView/types.ts — ListViewFilter does not declare an `inputName` property (no index signature either), so adding `inputName` on the object literal assigned to ListViewFilters will trigger TypeScript's excess property checks and fail compilation. There's also no usage of `inputName` elsewhere in the codebase (search for "inputName" returned no matches), so removing this line is the correct and minimal fix. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/RowLevelSecurityList/index.tsx **Line:** 269:269 **Comment:** *Type Error: The added `inputName` property on the filter config is not part of the `ListViewFilter` interface (as defined in `src/components/ListView/types.ts`), so TypeScript's excess property checks will reject this object literal and prevent this file from compiling; since nothing in the ListView/Filters/Search pipeline consumes `inputName`, the safest fix in this file is to remove the property. 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/components/ListView/Filters/index.test.tsx: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ListViewFilterOperator } from '../types'; +import UIFilters from './index'; + +const mockUpdateFilterValue = jest.fn(); + +beforeEach(() => { + mockUpdateFilterValue.mockClear(); +}); + +test('search filter uses id as input name when inputName is not provided', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('name'); +}); + +test('search filter uses inputName when provided instead of id', () => { + const filters = [ + { + Header: 'Name', + key: 'name', + id: 'name', + input: 'search' as const, + operator: ListViewFilterOperator.Contains, + inputName: 'custom_search_name', + }, + ]; + + render( + <UIFilters + filters={filters} + internalFilters={[]} + updateFilterValue={mockUpdateFilterValue} + />, + ); + + const input = screen.getByTestId('filters-search') as HTMLInputElement; + expect(input.name).toBe('custom_search_name'); Review Comment: **Suggestion:** This test assumes that providing `inputName` in the filter configuration changes the underlying input's `name` attribute, but the current `UIFilters` implementation still uses the filter `id` as the `name`, so this expectation will consistently fail; the test should assert the actual contract that `name` comes from `id`. [logic error] **Severity Level:** Minor ⚠️ ```suggestion test('search filter uses id as input name even when an unused inputName is provided', () => { const filters = [ { Header: 'Name', key: 'name', id: 'name', input: 'search' as const, operator: ListViewFilterOperator.Contains, inputName: 'custom_search_name', }, ]; render( <UIFilters filters={filters} internalFilters={[]} updateFilterValue={mockUpdateFilterValue} />, ); const input = screen.getByTestId('filters-search') as HTMLInputElement; expect(input.name).toBe('name'); ``` <details> <summary><b>Why it matters? ⭐ </b></summary> I inspected UIFilters and SearchFilter: UIFilters passes name={id} to SearchFilter (index.tsx line where SearchFilter gets name={id}), and SearchFilter renders <Input name={name} ... /> (Search.tsx). There is no handling of an inputName property on the filter objects or propagation of autoComplete. Therefore the test as written expects behavior that the component does not implement — changing the test to assert the actual contract (name comes from id) fixes a real test assertion mismatch rather than being a pointless style 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/components/ListView/Filters/index.test.tsx **Line:** 52:73 **Comment:** *Logic Error: This test assumes that providing `inputName` in the filter configuration changes the underlying input's `name` attribute, but the current `UIFilters` implementation still uses the filter `id` as the `name`, so this expectation will consistently fail; the test should assert the actual contract that `name` comes from `id`. 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]
