codeant-ai-for-open-source[bot] commented on code in PR #39068: URL: https://github.com/apache/superset/pull/39068#discussion_r3407205094
########## superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts: ########## @@ -0,0 +1,105 @@ +/** + * 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 { + splitWithQuoteEscaping, + stripSurroundingQuotes, + findLastUnquotedSeparatorIndex, +} from './utils'; + +test('stripSurroundingQuotes removes matching surrounding double quotes', () => { + expect(stripSurroundingQuotes('"foo"')).toBe('foo'); +}); + +test('stripSurroundingQuotes trims whitespace before stripping', () => { + expect(stripSurroundingQuotes(' "foo" ')).toBe('foo'); +}); + +test('stripSurroundingQuotes leaves text untouched when only one side is quoted', () => { + expect(stripSurroundingQuotes('"foo')).toBe('"foo'); + expect(stripSurroundingQuotes('foo"')).toBe('foo"'); +}); + +test('stripSurroundingQuotes preserves quotes in the middle of the string', () => { + expect(stripSurroundingQuotes('a"b"c')).toBe('a"b"c'); +}); + +test('stripSurroundingQuotes returns an empty string for `""`', () => { + expect(stripSurroundingQuotes('""')).toBe(''); +}); + +test('stripSurroundingQuotes does not strip a single lone quote', () => { + expect(stripSurroundingQuotes('"')).toBe('"'); +}); + +test('stripSurroundingQuotes returns an empty string for empty/whitespace input', () => { + expect(stripSurroundingQuotes('')).toBe(''); + expect(stripSurroundingQuotes(' ')).toBe(''); +}); + +test('splitWithQuoteEscaping splits by separator and trims tokens', () => { + expect(splitWithQuoteEscaping('a, b ,c', [','])).toEqual(['a', 'b', 'c']); +}); + +test('splitWithQuoteEscaping preserves separator characters inside double quotes', () => { + expect(splitWithQuoteEscaping('"a, b",c', [','])).toEqual(['a, b', 'c']); +}); + +test('splitWithQuoteEscaping strips quote characters from the output tokens', () => { + expect(splitWithQuoteEscaping('"foo","bar"', [','])).toEqual(['foo', 'bar']); +}); + +test('splitWithQuoteEscaping drops empty tokens from adjacent or edge separators', () => { + expect(splitWithQuoteEscaping('a,,b', [','])).toEqual(['a', 'b']); + expect(splitWithQuoteEscaping(',a,b,', [','])).toEqual(['a', 'b']); +}); + +test('splitWithQuoteEscaping treats unclosed quotes as opening a region that runs to end', () => { + expect(splitWithQuoteEscaping('"a, b', [','])).toEqual(['a, b']); +}); + +test('splitWithQuoteEscaping returns the original text when no separator is present', () => { + expect(splitWithQuoteEscaping('hello world', [','])).toEqual(['hello world']); +}); Review Comment: **Suggestion:** This assertion locks in non-trimmed behavior when there is no separator, but the tokenization utility is also used for paste flows where users often include trailing spaces. Keeping surrounding spaces in a single-token paste causes option matching to fail and can create incorrect new values (e.g. `"Canada "` instead of `Canada`). Update the expected behavior to trim even the single-token case. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Native select filters fail to apply pasted values. - ⚠️ Async selects create whitespace-padded options for pasted items. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a dashboard native select filter which uses the `Select` component from `@superset-ui/core/components` via `SelectFilterPlugin` at `superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:11-44`, with `multiSelect` set so that `mode={multiSelect ? 'multiple' : 'single'}` is `'multiple'` (line ~26). 2. In the running UI, paste a single value with a trailing space (e.g. `Canada `) into the multi-select input for that filter; this triggers the `onPaste` handler in `Select.tsx` at `superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:751-56`, which because `isSingleMode` is false executes the multi-select branch, calls `e.preventDefault()`, and computes `array = uniq(splitWithQuoteEscaping(pastedText, tokenSeparators))`. 3. In `splitWithQuoteEscaping` (implemented in `superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:264-95`), since `pastedText` (`"Canada "`) contains none of the configured `TOKEN_SEPARATORS` from `constants.ts:26` (`[',', '\r\n', '\n', '\t', ';']`), `const separator = separators.find(sep => text.includes(sep))` returns `undefined` and the function immediately returns `[text]` without trimming, as codified by the test at `utils.test.ts:76-78`. 4. Back in `Select.tsx` `onPaste`, that untrimmed value `"Canada "` is passed to `getPastedTextValue` at `Select.tsx:720-759`, which looks up an option with `getOption(text, fullSelectOptions, true)` and, when no exact match is found and `allowNewOptions` is false (the default for many filters), returns `undefined`; the result is that the paste operation silently selects nothing (or, when `allowNewOptions` is true, creates a new option with the incorrect label `"Canada "`) purely because the single-token case kept surrounding whitespace per the test expectation. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a77db9620e794f1a9c94fe7e3d578765&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a77db9620e794f1a9c94fe7e3d578765&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts **Line:** 76:78 **Comment:** *Logic Error: This assertion locks in non-trimmed behavior when there is no separator, but the tokenization utility is also used for paste flows where users often include trailing spaces. Keeping surrounding spaces in a single-token paste causes option matching to fail and can create incorrect new values (e.g. `"Canada "` instead of `Canada`). Update the expected behavior to trim even the single-token case. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=731696c326f2cde109ec32c637493e2daa22d50af646d06ee0a58bddb87924b7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=731696c326f2cde109ec32c637493e2daa22d50af646d06ee0a58bddb87924b7&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts: ########## @@ -0,0 +1,105 @@ +/** + * 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 { + splitWithQuoteEscaping, + stripSurroundingQuotes, + findLastUnquotedSeparatorIndex, +} from './utils'; + +test('stripSurroundingQuotes removes matching surrounding double quotes', () => { + expect(stripSurroundingQuotes('"foo"')).toBe('foo'); +}); + +test('stripSurroundingQuotes trims whitespace before stripping', () => { + expect(stripSurroundingQuotes(' "foo" ')).toBe('foo'); +}); + +test('stripSurroundingQuotes leaves text untouched when only one side is quoted', () => { + expect(stripSurroundingQuotes('"foo')).toBe('"foo'); + expect(stripSurroundingQuotes('foo"')).toBe('foo"'); +}); + +test('stripSurroundingQuotes preserves quotes in the middle of the string', () => { + expect(stripSurroundingQuotes('a"b"c')).toBe('a"b"c'); +}); + +test('stripSurroundingQuotes returns an empty string for `""`', () => { + expect(stripSurroundingQuotes('""')).toBe(''); +}); + +test('stripSurroundingQuotes does not strip a single lone quote', () => { + expect(stripSurroundingQuotes('"')).toBe('"'); +}); + +test('stripSurroundingQuotes returns an empty string for empty/whitespace input', () => { + expect(stripSurroundingQuotes('')).toBe(''); + expect(stripSurroundingQuotes(' ')).toBe(''); +}); + +test('splitWithQuoteEscaping splits by separator and trims tokens', () => { + expect(splitWithQuoteEscaping('a, b ,c', [','])).toEqual(['a', 'b', 'c']); +}); + +test('splitWithQuoteEscaping preserves separator characters inside double quotes', () => { + expect(splitWithQuoteEscaping('"a, b",c', [','])).toEqual(['a, b', 'c']); +}); + +test('splitWithQuoteEscaping strips quote characters from the output tokens', () => { + expect(splitWithQuoteEscaping('"foo","bar"', [','])).toEqual(['foo', 'bar']); +}); + +test('splitWithQuoteEscaping drops empty tokens from adjacent or edge separators', () => { + expect(splitWithQuoteEscaping('a,,b', [','])).toEqual(['a', 'b']); + expect(splitWithQuoteEscaping(',a,b,', [','])).toEqual(['a', 'b']); +}); + +test('splitWithQuoteEscaping treats unclosed quotes as opening a region that runs to end', () => { + expect(splitWithQuoteEscaping('"a, b', [','])).toEqual(['a, b']); +}); + +test('splitWithQuoteEscaping returns the original text when no separator is present', () => { + expect(splitWithQuoteEscaping('hello world', [','])).toEqual(['hello world']); +}); + +test('splitWithQuoteEscaping uses the first separator from the list that appears in the text', () => { + expect(splitWithQuoteEscaping('a;b,c', [',', ';'])).toEqual(['a;b', 'c']); + expect(splitWithQuoteEscaping('a;b', [',', ';'])).toEqual(['a', 'b']); +}); Review Comment: **Suggestion:** This test codifies splitting by only one separator chosen from the list, which is inconsistent with token separator semantics used by the Select components (multiple separators are configured and expected to work together). With mixed-delimiter input (common in pasted data), values after the other separators are not tokenized. The expected behavior should validate splitting on all unquoted separators, not just the first matching separator type. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Multi-line pasted filter values collapse into a single token. - ⚠️ Users cannot paste quoted CSV lists into filters. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use a native multi-select filter (again via `SelectFilterPlugin` at `superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:11-44`) where `multiSelect` is true so `mode='multiple'`, and the underlying `Select` component from `superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx` is used. 2. In a spreadsheet or CSV with two rows `"Australia, NZ"` and `Canada`, copy both rows and paste them into the filter; the clipboard text will look like `"Australia, NZ"\nCanada`, containing both a comma and a newline, and triggers `onPaste` in `Select.tsx` at `:751-56`, which in the multi-select branch executes `const array = uniq(splitWithQuoteEscaping(pastedText, tokenSeparators))` with `tokenSeparators` defaulting to `TOKEN_SEPARATORS` from `constants.ts:26` (`[',', '\r\n', '\n', '\t', ';']`). 3. Inside `splitWithQuoteEscaping` in `superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:264-95`, `const separator = separators.find(sep => text.includes(sep))` chooses the first matching separator type from the list—here `','` because the pasted text contains a comma—so only commas are considered delimiters; the loop toggles `inQuotes` on `"` characters and never treats the newline as a separator because it only checks `text.startsWith(separator, i)` for the chosen `','`. 4. As a result, the entire string is accumulated into a single token like `'Australia, NZ\nCanada'` (quotes stripped by the algorithm's handling of `"`) instead of two tokens, and `onPaste` then passes just that single combined value through `getPastedTextValue` in `Select.tsx:720-759`, which cannot match existing options `Australia, NZ` and `Canada`, so the pasted list is not correctly tokenized; this behavior is exactly locked in by the test at `utils.test.ts:80-83` that asserts only the first separator type is respected for splitting mixed-delimiter input. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6531cc67ea6541d88908ce4ea2773e62&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6531cc67ea6541d88908ce4ea2773e62&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts **Line:** 80:83 **Comment:** *Logic Error: This test codifies splitting by only one separator chosen from the list, which is inconsistent with token separator semantics used by the Select components (multiple separators are configured and expected to work together). With mixed-delimiter input (common in pasted data), values after the other separators are not tokenized. The expected behavior should validate splitting on all unquoted separators, not just the first matching separator type. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=240f292928fb21875f1bf1af186be2176e7010c6a35bbfffae2007f98089bfcc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=240f292928fb21875f1bf1af186be2176e7010c6a35bbfffae2007f98089bfcc&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: [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]
