codeant-ai-for-open-source[bot] commented on code in PR #41437: URL: https://github.com/apache/superset/pull/41437#discussion_r3785995077
########## superset-frontend/playwright/components/modals/DrillDetailModal.ts: ########## @@ -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 { Locator, Page } from '@playwright/test'; +import { Modal } from '../core'; + +/** + * The "Drill to detail" modal (`DrillDetailModal.tsx`), opened from a chart's + * "More Options" menu or its right-click context menu. Renders the chart's + * underlying sample rows, optionally scoped to a drilled-by value, via the + * `/datasource/samples` API. + */ +export class DrillDetailModal extends Modal { + private static readonly SELECTORS = { + CLOSE_BUTTON: '[data-test="close-drilltodetail-modal"]', + ROW_COUNT_LABEL: '[data-test="row-count-label"]', + METADATA_BAR: '[data-test="metadata-bar"]', + FILTER_COLUMN: '[data-test="filter-col"]', + FILTER_VALUE: '[data-test="filter-val"]', + PAGE_ITEM: '.ant-pagination-item', + ACTIVE_PAGE_ITEM: '.ant-pagination-item-active', + GRID_CELL: '.virtual-table-cell', + } as const; + + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.specificLocator = page.getByRole('dialog', { + name: /^Drill to detail:/, Review Comment: **Suggestion:** The dialog locator hard-codes the English prefix `Drill to detail:` even though the production modal sets its accessible name with the translated `t('Drill to detail: %s', chartName)`. In a non-English locale this locator never resolves, so every assertion and interaction through `drillModal()` fails. Use a locale-independent selector or obtain the localized accessible name from the page. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Drill-detail Playwright tests fail in non-English locales. - ⚠️ Localized CI loses modal regression coverage. - ⚠️ Modal assertions cannot observe valid production dialogs. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3549a55ba784458a8fe75324e93f627e&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=3549a55ba784458a8fe75324e93f627e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <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/DrillDetailModal.ts **Line:** 45:46 **Comment:** *Api Mismatch: The dialog locator hard-codes the English prefix `Drill to detail:` even though the production modal sets its accessible name with the translated `t('Drill to detail: %s', chartName)`. In a non-English locale this locator never resolves, so every assertion and interaction through `drillModal()` fails. Use a locale-independent selector or obtain the localized accessible name from the page. 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%2F41437&comment_hash=2959645bd10e63e43cca55d0b1d118962717f5d7941b11c40feda0b72f57494c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41437&comment_hash=2959645bd10e63e43cca55d0b1d118962717f5d7941b11c40feda0b72f57494c&reaction=dislike'>👎</a> ########## superset-frontend/playwright/pages/DashboardPage.ts: ########## @@ -454,4 +455,113 @@ export class DashboardPage { return { heightBefore: boxBefore.height, heightAfter: boxAfter.height }; } + + // --------------------------------------------------------------------------- + // Drill to detail + // + // Charts that implement the DRILL_TO_DETAIL behavior expose two entry points: + // the chart's "More Options" header menu, and a right-click context menu on + // the chart body (a cell, the big-number value, or a canvas data point). Both + // open the same DrillDetailModal, which renders the underlying sample rows for + // the (optionally filtered) chart by calling the `/datasource/samples` API. + // --------------------------------------------------------------------------- + + /** + * Open the "Drill to detail" item from a chart's "More Options" header menu. + * This is the whole-chart entry point (no row-level filters applied). + */ + async openDrillToDetailFromMenu(chartId: number): Promise<void> { + const moreOptions = new Button( + this.page, + this.getChart(chartId).getByLabel('More Options', { exact: true }), + ); + await moreOptions.click(); + await this.page + .getByRole('menuitem', { name: 'Drill to detail', exact: true }) + .click(); + } + + /** + * The DrillDetailModal dialog (titled "Drill to detail: <chart name>"). + */ + drillModal(): DrillDetailModal { + return new DrillDetailModal(this.page); + } + + /** + * Click the plain "Drill to detail" item in an open chart context menu + * (whole chart, no row-level filter). + */ + async contextMenuDrillToDetail(): Promise<void> { + await this.page + .getByRole('menuitem', { name: 'Drill to detail', exact: true }) + .click(); + } + + /** + * The "Drill to detail by" submenu parent (title) in an open context menu. + * Targeted by its submenu-title element rather than role+name because antd + * appends the arrow-icon name ("right") to the accessible name, and the leaf + * items ("Drill to detail by boy") would otherwise match a role+name lookup. + */ + drillBySubmenuTitle(): Locator { + return this.page.locator('.ant-dropdown-menu-submenu-title', { + hasText: 'Drill to detail by', + }); + } + + /** + * The chart context menu's Menu component, scoped to the open context + * menu's root. Used to open the "Drill to detail by" submenu robustly: + * plain hover is not reliably picked up by Ant Design's submenu trigger in + * headless Chromium, so this falls back to keyboard and dispatchEvent - see + * {@link Menu.openSubmenu}. + */ + private contextMenu(): Menu { + return new Menu(this.page, '[data-test="chart-context-menu"]'); + } + + /** + * Opens the "Drill to detail by" submenu and returns its popup, containing + * the leaf value items (e.g. "Drill to detail by boy"). + */ + private openDrillBySubmenu(): Promise<Locator> { + return this.contextMenu().openSubmenu('Drill to detail by', { + popupSelector: '.chart-context-submenu', + }); + } + + /** + * From an open chart context menu, open the "Drill to detail by" submenu and + * click the entry for a specific value (e.g. "boy", "1965", "all"). + */ + async contextMenuDrillToDetailBy(value: string): Promise<void> { + const popup = await this.openDrillBySubmenu(); + // Use dispatchEvent instead of click to bypass viewport and pointer + // interception issues - see Menu.selectSubmenuItem. + await popup + .getByRole('menuitem', { + name: `Drill to detail by ${value}`, + exact: true, + }) + .dispatchEvent('click'); Review Comment: **Suggestion:** The submenu item is located by interpolating the visible text returned by `allInnerTexts()` into an accessible-name query. The production menu sets `aria-label` from the raw formatted value, while its displayed child strips HTML tags; for a formatted value containing markup, the extracted visible value differs from the accessible name and this exact locator finds no item even though it was just offered. Select the item using the actual submenu item text/attribute or preserve and use the accessible label returned by the menu. [api mismatch] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ❌ Formatted-value drill tests cannot select offered items. - ⚠️ Canvas drill round-trip coverage fails for HTML-formatted values. - ⚠️ Valid drill menu entries become unselectable by the page object. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=776ad3bb24b040a98d0a7b0e5948ae85&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=776ad3bb24b040a98d0a7b0e5948ae85&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/pages/DashboardPage.ts **Line:** 542:547 **Comment:** *Api Mismatch: The submenu item is located by interpolating the visible text returned by `allInnerTexts()` into an accessible-name query. The production menu sets `aria-label` from the raw formatted value, while its displayed child strips HTML tags; for a formatted value containing markup, the extracted visible value differs from the accessible name and this exact locator finds no item even though it was just offered. Select the item using the actual submenu item text/attribute or preserve and use the accessible label returned by the menu. 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%2F41437&comment_hash=e69cea3e836eb4bd4c46f827d6f30eaee0ae5b60108f3bfc9ac4f71400fe6209&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41437&comment_hash=e69cea3e836eb4bd4c46f827d6f30eaee0ae5b60108f3bfc9ac4f71400fe6209&reaction=dislike'>👎</a> ########## superset-frontend/playwright/tests/dashboard/dashboard-drill-to-detail.spec.ts: ########## @@ -0,0 +1,747 @@ +/** + * 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. + */ + +/** + * E2E migration of the Cypress "Drill to detail modal" suite + * (dashboard/drilltodetail.test.ts). + * + * Drill to detail lets a viewer open a modal of the underlying sample rows for a + * chart — optionally filtered to a single data point — by either the chart's + * "More Options" header menu or a right-click context menu on the chart body. + * The modal calls the real `/datasource/samples` API, so this is genuinely + * end-to-end: each test API-builds a hermetic dashboard from the `birth_names` + * dataset, renders it in the browser, drives the real menus, and asserts the + * resulting backend round-trip (the samples POST and the filter the modal + * applies). + * + * Why the original suite was fully `describe.skip`: + * "it has issues with autoscrolling and the locked title flakes intricately + * when the rightClick is obstructed by the title." + * That failure mode is Cypress-specific — Cypress auto-scrolls the target under + * the sticky chart header before every action. Playwright scrolls once and the + * target stays put, so the entry points are portable here. + * + * What is migrated, and how it is kept deterministic: + * - Modal mechanics (open from header menu, pagination, reload-resets-page) + * and the no-filter big-number drill use stable DOM elements. + * - Table and Pivot drills right-click real DOM cells (no canvas pixels). + * - Canvas (echarts) charts — Pie, Line, Scatter, generic/smooth/step + * time-series, Mixed, Box plot, Funnel, Gauge, Treemap — DID rely on + * hard-coded pixel coordinates in Cypress to land on a specific slice/point. + * Instead of reproducing those brittle pixels, these tests scan a stable + * region of the canvas (see `rightClickCanvasDatum`), read whichever value + * the drill submenu actually offers for the point under the cursor, drill by + * that value, and assert the SAME value round-trips into the modal filter. + * This exercises the full canvas → contextmenu → datum → samples pipeline + * while staying independent of exact geometry. `Big Number with Trendline` + * drills the whole chart (no datum filter), like `Big Number`. + * + * Excluded (kept out, matching the original's own `describe.skip`s): Bar, Area, + * World Map, Radar — skipped upstream for chart-specific reasons. + */ +import { + testWithAssets, + expect, + type TestAssets, +} from '../../helpers/fixtures'; +import type { Page, TestInfo } from '@playwright/test'; +import { TIMEOUT } from '../../utils/constants'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { createDashboardWithCharts } from './dashboard-test-helpers'; + +const DATASET_NAME = 'birth_names'; + +/** + * Parse a RowCountLabel value ("75.7k rows", "1,234 rows") into a number so + * tests can assert the *invariant* (filtered < unfiltered) without hard-coding + * the dataset-specific totals the original Cypress suite baked in. + */ +function parseRowCount(text: string): number { + const m = text.match(/([\d.,]+)\s*([kKmM]?)/); + if (!m) return NaN; + let n = parseFloat(m[1].replace(/,/g, '')); Review Comment: **Suggestion:** The parser removes every comma before calling `parseFloat`, so a localized count such as `75,7k rows` is interpreted as `757k` instead of `75.7k`. The filtered/unfiltered row-count assertions therefore compare incorrect values and can fail or lose their intended regression-detection behavior in locales using a decimal comma. Parse the locale-formatted value consistently with the number formatter, or assert against the raw numeric value. [type error] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Filtered row-count assertions use incorrect numeric values. - ⚠️ Locale-specific drill regression checks can become misleading. - ⚠️ Table filter-size validation loses numeric accuracy. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57ffcd7f4b2542648840ec517a9600a4&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=57ffcd7f4b2542648840ec517a9600a4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/playwright/tests/dashboard/dashboard-drill-to-detail.spec.ts **Line:** 78:78 **Comment:** *Type Error: The parser removes every comma before calling `parseFloat`, so a localized count such as `75,7k rows` is interpreted as `757k` instead of `75.7k`. The filtered/unfiltered row-count assertions therefore compare incorrect values and can fail or lose their intended regression-detection behavior in locales using a decimal comma. Parse the locale-formatted value consistently with the number formatter, or assert against the raw numeric value. 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%2F41437&comment_hash=16f972c0cc1a5b468dc692b1bc7d81f72c163facf3ca89517097574a878ad77a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41437&comment_hash=16f972c0cc1a5b468dc692b1bc7d81f72c163facf3ca89517097574a878ad77a&reaction=dislike'>👎</a> ########## superset-frontend/playwright/components/modals/DrillDetailModal.ts: ########## @@ -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 { Locator, Page } from '@playwright/test'; +import { Modal } from '../core'; + +/** + * The "Drill to detail" modal (`DrillDetailModal.tsx`), opened from a chart's + * "More Options" menu or its right-click context menu. Renders the chart's + * underlying sample rows, optionally scoped to a drilled-by value, via the + * `/datasource/samples` API. + */ +export class DrillDetailModal extends Modal { + private static readonly SELECTORS = { + CLOSE_BUTTON: '[data-test="close-drilltodetail-modal"]', + ROW_COUNT_LABEL: '[data-test="row-count-label"]', + METADATA_BAR: '[data-test="metadata-bar"]', + FILTER_COLUMN: '[data-test="filter-col"]', + FILTER_VALUE: '[data-test="filter-val"]', + PAGE_ITEM: '.ant-pagination-item', + ACTIVE_PAGE_ITEM: '.ant-pagination-item-active', + GRID_CELL: '.virtual-table-cell', + } as const; + + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.specificLocator = page.getByRole('dialog', { + name: /^Drill to detail:/, + }); + } + + override get element(): Locator { + return this.specificLocator; + } + + /** + * The applied-filter value tags (`<col>=<val>`). Empty when the drill was + * whole-chart (no row/point-level filter applied). + */ + get filterValues(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.FILTER_VALUE); + } + + /** The applied-filter chip(s); each is closable via its own "Close" icon. */ + get filterColumns(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.FILTER_COLUMN); + } + + /** Row-count label above the results grid, e.g. "1-50 of 500 rows". */ + get rowCountLabel(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.ROW_COUNT_LABEL); + } + + /** The metadata bar (column/row summary) shown once samples have loaded. */ + get metadataBar(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.METADATA_BAR); + } + + /** Pagination page-number items below the results grid. */ + get pageItems(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.PAGE_ITEM); + } + + /** The currently active pagination page-number item. */ + get activePageItem(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.ACTIVE_PAGE_ITEM); + } + + /** Cells of the virtualized results grid. */ + get gridCells(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.GRID_CELL); + } + + /** + * Removes the first applied filter by clicking its chip's Close icon, + * re-fetching the unfiltered samples. + */ + async clearFirstFilter(): Promise<void> { + await this.filterColumns.first().getByLabel('Close').click(); + } + + /** Navigates to the given 1-indexed pagination page. */ + async goToPage(pageNumber: number): Promise<void> { + await this.pageItems.nth(pageNumber - 1).click(); + } + + /** Re-fetches the current samples query, resetting pagination to page 1. */ + async reload(): Promise<void> { + await this.element.getByRole('button', { name: 'Reload' }).click(); Review Comment: **Suggestion:** The reload control's accessible name is produced by `t('Reload')`, but this method searches for the literal English name `Reload`. Consequently, clicking reload fails in localized deployments and the pagination-reset test cannot exercise the control. Use a stable selector or a locale-aware lookup. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Reload-reset coverage fails in localized test runs. - ⚠️ Pagination state is not verified after reload. - ⚠️ `handleReload` is never exercised by the migrated test. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b595cc26e3ff47e5ba19d0c2e8a73116&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=b595cc26e3ff47e5ba19d0c2e8a73116&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <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/DrillDetailModal.ts **Line:** 107:107 **Comment:** *Api Mismatch: The reload control's accessible name is produced by `t('Reload')`, but this method searches for the literal English name `Reload`. Consequently, clicking reload fails in localized deployments and the pagination-reset test cannot exercise the control. Use a stable selector or a locale-aware lookup. 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%2F41437&comment_hash=d8e8e96e9b88436e43791e9b11050bc78352c715c6812b6fda7d439a4647b3d3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41437&comment_hash=d8e8e96e9b88436e43791e9b11050bc78352c715c6812b6fda7d439a4647b3d3&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]
