bito-code-review[bot] commented on code in PR #36684: URL: https://github.com/apache/superset/pull/36684#discussion_r2760513195
########## superset-frontend/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect cursor position in setValue</b></div> <div id="fix"> In setText, editor.setValue(value, 1) places the cursor after the first character instead of at the end. Omit the second argument to place it at the end by default. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const editor = (window as any).ace.edit(id); editor.setValue(value); editor.session.getUndoManager().reset(); }, ```` </div> </details> </div> <small><i>Code Review Run #02fb44</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/playwright/pages/ChartCreationPage.ts: ########## @@ -0,0 +1,138 @@ +/** + * 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 { expect, Locator, Page } from '@playwright/test'; +import { Button, Select } from '../components/core'; + +/** + * Chart Creation Page object for the "Create a new chart" wizard. + * This page appears after creating a dataset via the wizard. + */ +export class ChartCreationPage { + readonly page: Page; + + private static readonly SELECTORS = { + VIZ_GALLERY: '.viz-gallery', + VIZ_TYPE_ITEM: '[data-test="viz-type-gallery__item"]', + } as const; + + constructor(page: Page) { + this.page = page; + } + + /** + * Gets the dataset selector container (includes the displayed selection value) + */ + getDatasetSelectContainer(): Locator { + return this.page.getByLabel('Dataset', { exact: false }).first(); + } + + /** + * Gets the dataset selector for interactions + */ + getDatasetSelect(): Select { + return new Select( + this.page, + this.page.getByRole('combobox', { name: /dataset/i }), + ); + } + + /** + * Gets the visualization gallery container + */ + getVizGallery(): Locator { + return this.page.locator(ChartCreationPage.SELECTORS.VIZ_GALLERY); + } + + /** + * Gets the "Create new chart" button + */ + getCreateChartButton(): Button { + return new Button( + this.page, + this.page.getByRole('button', { name: /create new chart/i }), + ); + } + + /** + * Navigate to the chart creation page + */ + async goto(): Promise<void> { + await this.page.goto('chart/add'); + } + + /** + * Wait for the page to load (dataset selector visible) + */ + async waitForPageLoad(): Promise<void> { + await expect(this.getDatasetSelect().element).toBeVisible({ + timeout: 10000, + }); + } + + /** + * Select a dataset from the dropdown + * @param datasetName - The name of the dataset to select + */ + async selectDataset(datasetName: string): Promise<void> { + await this.getDatasetSelect().selectOption(datasetName); + } + + /** + * Select a visualization type from the gallery + * @param vizType - The visualization type to select (e.g., 'Table', 'Bar Chart') + */ + async selectVizType(vizType: string): Promise<void> { + const vizGallery = this.getVizGallery(); + await expect(vizGallery).toBeVisible(); + + // Button names in the gallery are duplicated (e.g., "Table Table", "Bar Chart Bar Chart") + // because they include both the image alt text and the label text. + // Use exact match with the duplicated pattern to avoid matching similar names. + const vizTypeItem = vizGallery.getByRole('button', { + name: `${vizType} ${vizType}`, + exact: true, + }); + await vizTypeItem.click(); + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect Button Selector Logic</b></div> <div id="fix"> The selectVizType method incorrectly assumes button accessible names are duplicated (e.g., 'Table Table') by concatenating image alt text and label text. However, for div elements with role='button', the accessible name is solely the text content, which is just the label (type.name). This will cause the selector to fail finding the correct button. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - async selectVizType(vizType: string): Promise<void> { - const vizGallery = this.getVizGallery(); - await expect(vizGallery).toBeVisible(); - - // Button names in the gallery are duplicated (e.g., \"Table Table\", \"Bar Chart Bar Chart\") - // because they include both the image alt text and the label text. - // Use exact match with the duplicated pattern to avoid matching similar names. - const vizTypeItem = vizGallery.getByRole('button', { - name: `${vizType} ${vizType}`, - exact: true, - }); - await vizTypeItem.click(); - } + async selectVizType(vizType: string): Promise<void> { + const vizGallery = this.getVizGallery(); + await expect(vizGallery).toBeVisible(); - + // For a div with role=\"button\", the accessible name is the text content, which is just the label text. + const vizTypeItem = vizGallery.getByRole('button', { + name: vizType, - }); - await vizTypeItem.click(); - } ``` </div> </details> </div> <small><i>Code Review Run #02fb44</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/playwright/components/core/Select.ts: ########## @@ -0,0 +1,174 @@ +/** + * 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'; + +/** + * Ant Design Select component selectors + */ +const SELECT_SELECTORS = { + DROPDOWN: '.ant-select-dropdown', + OPTION: '.ant-select-item-option', + SEARCH_INPUT: '.ant-select-selection-search-input', + CLEAR: '.ant-select-clear', +} as const; + +/** + * Select component for Ant Design Select/Combobox interactions. + */ +export class Select { + readonly page: Page; + private readonly locator: Locator; + + constructor(page: Page, selector: string); + constructor(page: Page, locator: Locator); + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Creates a Select from a combobox role with the given accessible name + * @param page - The Playwright page + * @param name - The accessible name (aria-label or placeholder text) + */ + static fromRole(page: Page, name: string): Select { + const locator = page.getByRole('combobox', { name }); + return new Select(page, locator); + } + + /** + * Gets the select element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Opens the dropdown, types to filter, and selects an option. + * Handles cases where the option may not be initially visible in the dropdown. + * Waits for dropdown to close after selection to avoid stale dropdowns. + * @param optionText - The text of the option to select + */ + async selectOption(optionText: string): Promise<void> { + await this.open(); + await this.type(optionText); + await this.clickOption(optionText); + // Wait for dropdown to close to avoid multiple visible dropdowns + await this.waitForDropdownClose(); + } + + /** + * Waits for dropdown to close after selection + * This prevents strict mode violations when multiple selects are used sequentially + */ + private async waitForDropdownClose(): Promise<void> { + // Wait for dropdown animation to complete + await this.page.waitForTimeout(300); + } + + /** + * Opens the dropdown + */ + async open(): Promise<void> { + await this.locator.click(); + } + + /** + * Clicks an option in an already-open dropdown by its text content. + * Uses selector-based approach matching Cypress patterns. + * Handles multiple dropdowns by targeting only visible, non-hidden ones. + * @param optionText - The text of the option to click (partial match for filtered results) + */ + async clickOption(optionText: string): Promise<void> { + // Target visible dropdown (excludes hidden ones via :not(.ant-select-dropdown-hidden)) + // Use .last() in case multiple dropdowns exist - the most recent one is what we want + const dropdown = this.page + .locator(`${SELECT_SELECTORS.DROPDOWN}:not(.ant-select-dropdown-hidden)`) + .last(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Potential wrong dropdown selection</b></div> <div id="fix"> Using .last() on the dropdown locator assumes only one select is open, but if multiple selects exist on the page, it may target the wrong dropdown and select incorrect options. </div> </div> <small><i>Code Review Run #02fb44</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/playwright/components/core/AceEditor.ts: ########## @@ -0,0 +1,152 @@ +/** + * 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'; + +const ACE_EDITOR_SELECTORS = { + TEXT_INPUT: '.ace_text-input', + TEXT_LAYER: '.ace_text-layer', + CONTENT: '.ace_content', + SCROLLER: '.ace_scroller', +} as const; + +/** + * AceEditor component for interacting with Ace Editor instances in Playwright. + * Uses the ace editor API directly for reliable text manipulation. + */ +export class AceEditor { + readonly page: Page; + private readonly selector: string; + private readonly locator: Locator; + + constructor(page: Page, selector: string) { + this.page = page; + this.selector = selector; + this.locator = page.locator(selector); + } + + /** + * Gets the editor element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Waits for the ace editor to be fully loaded and ready for interaction. + */ + async waitForReady(): Promise<void> { + await this.locator.waitFor({ state: 'visible' }); + await this.locator.locator(ACE_EDITOR_SELECTORS.CONTENT).waitFor(); + await this.locator.locator(ACE_EDITOR_SELECTORS.TEXT_LAYER).waitFor(); + } + + /** + * Sets text in the ace editor using the ace API. + * @param text - The text to set + */ + async setText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + editor.setValue(value, 1); + editor.session.getUndoManager().reset(); + }, + { id: editorId, value: text }, + ); + } + + /** + * Gets the text content from the ace editor. + * @returns The text content + */ + async getText(): Promise<string> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + return this.page.evaluate(id => { + const editor = (window as any).ace.edit(id); + return editor.getValue(); + }, editorId); + } + + /** + * Clears the text in the ace editor. + */ + async clear(): Promise<void> { + await this.setText(''); + } + + /** + * Appends text to the existing content in the ace editor. + * @param text - The text to append + */ + async appendText(text: string): Promise<void> { + await this.waitForReady(); + const editorId = this.extractEditorId(); + await this.page.evaluate( + ({ id, value }) => { + const editor = (window as any).ace.edit(id); + const currentText = editor.getValue(); + const newText = currentText + (currentText ? '\n' : '') + value; + editor.setValue(newText, 1); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect cursor position in appendText</b></div> <div id="fix"> In appendText, editor.setValue(newText, 1) also places the cursor incorrectly. Remove the , 1 to default to end. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion editor.setValue(newText); ```` </div> </details> </div> <small><i>Code Review Run #02fb44</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/playwright/components/modals/EditDatasetModal.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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 { Input, Modal, Tabs } from '../core'; + +/** + * Edit Dataset Modal component (DatasourceModal). + * Used for editing dataset properties like description, metrics, columns, etc. + * Uses specific dialog name to avoid strict mode violations when multiple dialogs are open. + */ +export class EditDatasetModal extends Modal { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing Test-Supporting Methods</b></div> <div id="fix"> The class lacks fillDescription and getDescription methods expected by the test in dataset-list.spec.ts, which will cause runtime errors during test execution. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - import { Input, Modal, Tabs } from '../core'; + import { Input, Modal, Tabs, Textarea } from '../core'; @@ -29,3 +29,4 @@ - private static readonly SELECTORS = { - NAME_INPUT: '[data-test="inline-name"]', - LOCK_ICON: '[data-test="lock"]', - UNLOCK_ICON: '[data-test="unlock"]', - }; + private static readonly SELECTORS = { + NAME_INPUT: '[data-test="inline-name"]', + DESCRIPTION: 'textarea[name="description"]', + LOCK_ICON: '[data-test="lock"]', + UNLOCK_ICON: '[data-test="unlock"]', + }; @@ -84,0 +85,15 @@ + /** + * Gets the dataset description textarea component + */ + private get descriptionTextarea(): Textarea { + return new Textarea( + this.page, + this.body.locator(EditDatasetModal.SELECTORS.DESCRIPTION), + ); + } + + /** + * Fill in the dataset description field + * @param description - The new dataset description + */ + async fillDescription(description: string): Promise<void> { + await this.descriptionTextarea.fill(description); + } + + /** + * Get the current dataset description + */ + async getDescription(): Promise<string> { + return this.descriptionTextarea.getValue(); + } ``` </div> </details> </div> <small><i>Code Review Run #02fb44</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/playwright/components/core/Select.ts: ########## @@ -0,0 +1,174 @@ +/** + * 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'; + +/** + * Ant Design Select component selectors + */ +const SELECT_SELECTORS = { + DROPDOWN: '.ant-select-dropdown', + OPTION: '.ant-select-item-option', + SEARCH_INPUT: '.ant-select-selection-search-input', + CLEAR: '.ant-select-clear', +} as const; + +/** + * Select component for Ant Design Select/Combobox interactions. + */ +export class Select { + readonly page: Page; + private readonly locator: Locator; + + constructor(page: Page, selector: string); + constructor(page: Page, locator: Locator); + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Creates a Select from a combobox role with the given accessible name + * @param page - The Playwright page + * @param name - The accessible name (aria-label or placeholder text) + */ + static fromRole(page: Page, name: string): Select { + const locator = page.getByRole('combobox', { name }); + return new Select(page, locator); + } + + /** + * Gets the select element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Opens the dropdown, types to filter, and selects an option. + * Handles cases where the option may not be initially visible in the dropdown. + * Waits for dropdown to close after selection to avoid stale dropdowns. + * @param optionText - The text of the option to select + */ + async selectOption(optionText: string): Promise<void> { + await this.open(); + await this.type(optionText); + await this.clickOption(optionText); + // Wait for dropdown to close to avoid multiple visible dropdowns + await this.waitForDropdownClose(); + } + + /** + * Waits for dropdown to close after selection + * This prevents strict mode violations when multiple selects are used sequentially + */ + private async waitForDropdownClose(): Promise<void> { + // Wait for dropdown animation to complete + await this.page.waitForTimeout(300); + } + + /** + * Opens the dropdown + */ + async open(): Promise<void> { + await this.locator.click(); + } + + /** + * Clicks an option in an already-open dropdown by its text content. + * Uses selector-based approach matching Cypress patterns. + * Handles multiple dropdowns by targeting only visible, non-hidden ones. + * @param optionText - The text of the option to click (partial match for filtered results) + */ + async clickOption(optionText: string): Promise<void> { + // Target visible dropdown (excludes hidden ones via :not(.ant-select-dropdown-hidden)) + // Use .last() in case multiple dropdowns exist - the most recent one is what we want + const dropdown = this.page + .locator(`${SELECT_SELECTORS.DROPDOWN}:not(.ant-select-dropdown-hidden)`) + .last(); + await dropdown.waitFor({ state: 'visible' }); + + // Find option by text content - use partial match since filtered results may have prefixes + // (e.g., searching for 'main' shows 'examples.main', 'system.main') + // First try exact match, fall back to partial match + const exactOption = dropdown + .locator(SELECT_SELECTORS.OPTION) + .getByText(optionText, { exact: true }); + + if ((await exactOption.count()) > 0) { + await exactOption.click(); + } else { + // Fall back to first option containing the text + const partialOption = dropdown + .locator(SELECT_SELECTORS.OPTION) + .filter({ hasText: optionText }) + .first(); + await partialOption.click(); + } + } + + /** + * Closes the dropdown by pressing Escape + */ + async close(): Promise<void> { + await this.page.keyboard.press('Escape'); + } + + /** + * Types into the select to filter options (assumes dropdown is open) + * @param text - The text to type + */ + async type(text: string): Promise<void> { + // Find the actual search input inside the select component + const searchInput = this.locator.locator(SELECT_SELECTORS.SEARCH_INPUT); + try { + // Wait for search input in case dropdown is still rendering + await searchInput.first().waitFor({ state: 'attached', timeout: 1000 }); + await searchInput.first().fill(text); + } catch { + // Fallback: locator might be the input itself (e.g., from getByRole('combobox')) + await this.locator.fill(text); + } + } + + /** + * Clears the current selection + */ + async clear(): Promise<void> { + await this.locator.clear(); + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect clear implementation</b></div> <div id="fix"> The clear() method calls locator.clear() on the select container, but Ant Design Select components require clicking the clear button instead. This will cause tests to fail when trying to clear selections. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion async clear(): Promise<void> { const clearButton = this.locator.locator(SELECT_SELECTORS.CLEAR); try { await clearButton.waitFor({ state: 'visible', timeout: 1000 }); await clearButton.click(); } catch { // Clear button not present, do nothing } } ```` </div> </details> </div> <small><i>Code Review Run #02fb44</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]
