codeant-ai-for-open-source[bot] commented on code in PR #42013: URL: https://github.com/apache/superset/pull/42013#discussion_r3574630053
########## superset-frontend/playwright/components/modals/NativeFiltersConfigModal.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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 { Locator, Page } from '@playwright/test'; +import { Modal } from '../core'; + +/** + * Native filters and Display Controls configuration modal. + */ +export class NativeFiltersConfigModal extends Modal { + private static readonly SELECTORS = { + MODAL: 'filter-modal', + SAVE_BUTTON: 'native-filter-modal-save-button', + } as const; + + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.specificLocator = page.getByTestId( + NativeFiltersConfigModal.SELECTORS.MODAL, + ); + } + + override get element(): Locator { + return this.specificLocator; + } + + /** + * Gets a Display Control row by name. + * @param name - The Display Control name + */ + private getDisplayControlRow(name: string): Locator { + return this.element.getByRole('tab').filter({ + has: this.page.getByText(name, { exact: true }), + }); + } + + /** + * Marks a Display Control for removal. + * @param name - The Display Control name + */ + async removeDisplayControl(name: string): Promise<void> { + const controlRow = this.getDisplayControlRow(name); + await controlRow.hover(); + await controlRow + .getByRole('button', { name: 'Remove customization' }) + .click(); Review Comment: **Suggestion:** The remove action is queried as a `button`, but the UI renders the delete control as an icon with an aria-label (not a button role), so this locator will fail to find it and the removal step will timeout. Query the control by label/test id (or the actual rendered role) instead of `getByRole('button', ...)`. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Dashboard delete-display-control Playwright test cannot remove item. - ❌ CI dashboard smoke suite fails on remove step. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Native Filters configuration modal in the dashboard UI, which renders the sidebar titles via `ItemTitleContainer` at `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.ts:127-188`. 2. Observe that the delete affordance is an `Icons.DeleteOutlined` icon at `ItemTitleContainer.ts:177-184` rendered inside a <div>, with only `aria-label={deleteAltText}` and no `role="button"` or <button> element. 3. In the Playwright layer, the modal wrapper `NativeFiltersConfigModal.removeDisplayControl()` at `superset-frontend/playwright/components/modals/NativeFiltersConfigModal.ts:59-64` locates the delete control using `controlRow.getByRole('button', { name: 'Remove customization' })`. 4. When a Playwright test calls `removeDisplayControl(name)` on a `NativeFiltersConfigModal` instance, Playwright searches for a role=button element named "Remove customization", finds none (because the icon has role img/none with just an aria-label), and the `.click()` call eventually times out, causing the removal step and the test to fail. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=93ab0c717bf54356b3b160d2f766c1b4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=93ab0c717bf54356b3b160d2f766c1b4&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/playwright/components/modals/NativeFiltersConfigModal.ts **Line:** 62:64 **Comment:** *Api Mismatch: The remove action is queried as a `button`, but the UI renders the delete control as an icon with an aria-label (not a button role), so this locator will fail to find it and the removal step will timeout. Query the control by label/test id (or the actual rendered role) instead of `getByRole('button', ...)`. 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%2F42013&comment_hash=6148948c1c3c2b51bd3ce6ab9db8559f137b80164d9a52755ec2caeddcd236d7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42013&comment_hash=6148948c1c3c2b51bd3ce6ab9db8559f137b80164d9a52755ec2caeddcd236d7&reaction=dislike'>👎</a> ########## superset-frontend/playwright/components/modals/NativeFiltersConfigModal.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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 { Locator, Page } from '@playwright/test'; +import { Modal } from '../core'; + +/** + * Native filters and Display Controls configuration modal. + */ +export class NativeFiltersConfigModal extends Modal { + private static readonly SELECTORS = { + MODAL: 'filter-modal', + SAVE_BUTTON: 'native-filter-modal-save-button', + } as const; + + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.specificLocator = page.getByTestId( + NativeFiltersConfigModal.SELECTORS.MODAL, + ); + } + + override get element(): Locator { + return this.specificLocator; + } + + /** + * Gets a Display Control row by name. + * @param name - The Display Control name + */ + private getDisplayControlRow(name: string): Locator { + return this.element.getByRole('tab').filter({ + has: this.page.getByText(name, { exact: true }), + }); + } + + /** + * Marks a Display Control for removal. + * @param name - The Display Control name + */ + async removeDisplayControl(name: string): Promise<void> { + const controlRow = this.getDisplayControlRow(name); + await controlRow.hover(); + await controlRow + .getByRole('button', { name: 'Remove customization' }) + .click(); + } + + /** + * Gets the marker shown for a removed Display Control. + * @param name - The Display Control name + */ + getRemovedMarker(name: string): Locator { + return this.getDisplayControlRow(name).getByText('(Removed)', { + exact: true, + }); + } Review Comment: **Suggestion:** After an item is removed, the sidebar title text is replaced by `(Removed)`, so re-locating the row by the original control name no longer matches and this method cannot find the removed marker. Use a locator that does not depend on the pre-removal name (or keep a stable row reference/id) before asserting the removed state. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Removed Display Control state cannot be asserted in tests. - ❌ Dashboard smoke test cannot verify customization removal marker. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the sidebar implementation, `ItemTitleContainer.renderComponent()` at `superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ItemTitleContainer.ts:127-188` renders each item title as an `ItemTitle` with `role="tab"` and text content. 2. When an item is marked removed (`isRemoved` truthy), the title text switches to `t('(Removed)')` at `ItemTitleContainer.ts:155`, so the original control name is no longer present anywhere in that tab row. 3. The Playwright helper `NativeFiltersConfigModal.getDisplayControlRow(name)` at `superset-frontend/playwright/components/modals/NativeFiltersConfigModal.ts:49-52` finds a row by `getByRole('tab').filter({ has: this.page.getByText(name, { exact: true }) })`, which requires the original name text to still exist inside the tab. 4. After calling `removeDisplayControl(name)`, if a test then calls `getRemovedMarker(name)`, `getDisplayControlRow(name)` matches no element (name text is gone), so the chained `.getByText('(Removed)'...)` has no base locator and cannot find the removed marker, making removed-state assertions fail even though the UI shows "(Removed)". ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=33b83141c60649db8aa56a9aca36f500&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=33b83141c60649db8aa56a9aca36f500&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/playwright/components/modals/NativeFiltersConfigModal.ts **Line:** 71:75 **Comment:** *Logic Error: After an item is removed, the sidebar title text is replaced by `(Removed)`, so re-locating the row by the original control name no longer matches and this method cannot find the removed marker. Use a locator that does not depend on the pre-removal name (or keep a stable row reference/id) before asserting the removed state. 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%2F42013&comment_hash=87790f4481c33b02ff4b18d285479919ddee1c43011f8e5cff8111ec2360fbf4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42013&comment_hash=87790f4481c33b02ff4b18d285479919ddee1c43011f8e5cff8111ec2360fbf4&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]
