fix(playwright): address review feedback on de-flake change

Three follow-ups from /review-code on the list-spec de-flake work:

- Revert toast assertions to expect(toast.getSuccess()).toBeVisible().
  The previous switch to toast.getSuccess().waitFor({ state: 'visible' })
  was a no-op: both APIs poll for visibility, so neither catches a fast
  auto-dismiss between polls. The expect() form fails as an assertion
  (clearer diff, counted in reports). Misleading "use waitFor so we
  detect auto-dismiss" comments are gone too.

- Drop redundant waitFor({ state: 'visible' }) calls in BulkSelect's
  selectRow / deselectRow / clickAction. Locator.check()/click() already
  auto-wait for visibility, stability, and enabled state. The
  expect(checkbox).toBeChecked() assertion -- which is the actual race
  guard against React state propagation -- stays.

- Expand the DeleteConfirmationModal.clickDelete docstring to explain
  why it bypasses Modal.clickFooterButton: the footer button label is
  i18n'd, so name-based lookups break in non-English locales.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

superset-frontend/playwright/components/ListView/BulkSelect.ts

@@ -17,12 +17,13 @@
  * under the License.
  */
 
-import { Locator, Page } from '@playwright/test';
+import { Locator, Page, expect } from '@playwright/test';
 import { Button, Checkbox, Table } from '../core';
 
 const BULK_SELECT_SELECTORS = {
   CONTROLS: '[data-test="bulk-select-controls"]',
   ACTION: '[data-test="bulk-select-action"]',
+  HEADER_TOGGLE: '[data-test="header-toggle-all"]',
 } as const;
 
 /**
@@ -56,10 +57,17 @@ export class BulkSelect {
   }
 
   /**
-   * Enables bulk selection mode by clicking the toggle button
+   * Enables bulk selection mode by clicking the toggle button.
+   *
+   * Waits for the bulk-select column header checkbox to render so the next
+   * row interaction does not race the table re-render that adds the
+   * checkbox column.
    */
   async enable(): Promise<void> {
     await this.getToggleButton().click();
+    await this.page
+      .locator(BULK_SELECT_SELECTORS.HEADER_TOGGLE)
+      .waitFor({ state: 'visible' });
   }
 
   /**
@@ -72,19 +80,27 @@ export class BulkSelect {
   }
 
   /**
-   * Selects a row's checkbox in bulk select mode
+   * Selects a row's checkbox in bulk select mode.
+   * Asserts the checkbox is checked afterwards so any state-update race
+   * surfaces here rather than as a missing bulk-action button later.
    * @param rowName - The name/text identifying the row to select
    */
   async selectRow(rowName: string): Promise<void> {
-    await this.getRowCheckbox(rowName).check();
+    const checkbox = this.getRowCheckbox(rowName);
+    await checkbox.check();
+    await expect(checkbox.element).toBeChecked();
   }
 
   /**
-   * Deselects a row's checkbox in bulk select mode
+   * Deselects a row's checkbox in bulk select mode.
+   * Mirrors selectRow: asserts the unchecked state so any lingering selection
+   * surfaces here rather than as a stale bulk-action count later.
    * @param rowName - The name/text identifying the row to deselect
    */
   async deselectRow(rowName: string): Promise<void> {
-    await this.getRowCheckbox(rowName).uncheck();
+    const checkbox = this.getRowCheckbox(rowName);
+    await checkbox.uncheck();
+    await expect(checkbox.element).not.toBeChecked();
   }
 
   /**
@@ -107,10 +123,11 @@ export class BulkSelect {
   }
 
   /**
-   * Clicks a bulk action button by name (e.g., "Export", "Delete")
+   * Clicks a bulk action button by name (e.g., "Export", "Delete").
    * @param actionName - The name of the bulk action to click
    */
   async clickAction(actionName: string): Promise<void> {
-    await this.getActionButton(actionName).click();
+    const button = this.getActionButton(actionName);
+    await button.click();
   }
 }

superset-frontend/playwright/components/modals/DeleteConfirmationModal.ts

@@ -17,6 +17,7 @@
  * under the License.
  */
 
+import { expect } from '@playwright/test';
 import { Modal, Input } from '../core';
 
 /**
@@ -27,7 +28,8 @@ import { Modal, Input } from '../core';
  */
 export class DeleteConfirmationModal extends Modal {
   private static readonly SELECTORS = {
-    CONFIRMATION_INPUT: 'input[type="text"]',
+    CONFIRMATION_INPUT: '[data-test="delete-modal-input"]',
+    CONFIRM_BUTTON: '[data-test="modal-confirm-button"]',
   };
 
   /**
@@ -36,12 +38,16 @@ export class DeleteConfirmationModal extends Modal {
   private get confirmationInput(): Input {
     return new Input(
       this.page,
-      this.body.locator(DeleteConfirmationModal.SELECTORS.CONFIRMATION_INPUT),
+      this.element.locator(
+        DeleteConfirmationModal.SELECTORS.CONFIRMATION_INPUT,
+      ),
     );
   }
 
   /**
    * Fills the confirmation input with the specified text.
+   * Waits for the input to be visible before filling so callers don't race
+   * with the modal's open animation / focus effect.
    *
    * @param confirmationText - The text to type
    * @param options - Optional fill options (timeout, force)
@@ -57,11 +63,25 @@ export class DeleteConfirmationModal extends Modal {
     confirmationText: string,
     options?: { timeout?: number; force?: boolean },
   ): Promise<void> {
+    await this.confirmationInput.element.waitFor({
+      state: 'visible',
+      timeout: options?.timeout,
+    });
     await this.confirmationInput.fill(confirmationText, options);
   }
 
   /**
-   * Clicks the Delete button in the footer
+   * Clicks the Delete button in the footer.
+   *
+   * Targets the confirm button by data-test rather than going through
+   * Modal.clickFooterButton, which finds buttons by their visible text. The
+   * button label is i18n'd ("Delete" / "Supprimer" / …) so name-based lookups
+   * break in non-English locales.
+   *
+   * Also waits for the button to become enabled before clicking: it is
+   * disabled until the confirmation text matches "DELETE", and React's state
+   * update from fillConfirmationInput is asynchronous, so an immediate click
+   * can race the disabled→enabled transition.
    *
    * @param options - Optional click options (timeout, force, delay)
    */
@@ -70,6 +90,10 @@ export class DeleteConfirmationModal extends Modal {
     force?: boolean;
     delay?: number;
   }): Promise<void> {
-    await this.clickFooterButton('Delete', options);
+    const confirmButton = this.element.locator(
+      DeleteConfirmationModal.SELECTORS.CONFIRM_BUTTON,
+    );
+    await expect(confirmButton).toBeEnabled({ timeout: options?.timeout });
+    await confirmButton.click(options);
   }
 }

superset-frontend/playwright/tests/chart/chart-list.spec.ts

@@ -32,6 +32,7 @@ import {
   expectStatusOneOf,
   expectValidExportZip,
 } from '../../helpers/api/assertions';
+import { TIMEOUT } from '../../utils/constants';
 
 /**
  * Extend testWithAssets with chartListPage navigation (beforeEach equivalent).
@@ -62,8 +63,11 @@ test('should delete a chart with confirmation', async ({
   await chartListPage.goto();
   await chartListPage.waitForTableLoad();
 
-  // Verify chart is visible in list
-  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created chart appears.
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Click delete action button
   await chartListPage.clickDeleteAction(chartName);
@@ -81,12 +85,12 @@ test('should delete a chart with confirmation', async ({
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
-  // Verify chart is removed from list
-  await expect(chartListPage.getChartRow(chartName)).not.toBeVisible();
+  // Verify chart is removed from list (deleted rows leave the DOM)
+  await expect(chartListPage.getChartRow(chartName)).toHaveCount(0);
 
   // Backend verification: API returns 404
   await expectDeleted(page, ENDPOINTS.CHART, chartId, {
@@ -111,8 +115,11 @@ test('should edit chart name via properties modal', async ({
   await chartListPage.goto();
   await chartListPage.waitForTableLoad();
 
-  // Verify chart is visible in list
-  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created chart appears.
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Click edit action to open properties modal
   await chartListPage.clickEditAction(chartName);
@@ -137,7 +144,7 @@ test('should edit chart name via properties modal', async ({
   // Modal should close
   await propertiesModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
@@ -164,8 +171,11 @@ test('should export a chart as a zip file', async ({
   await chartListPage.goto();
   await chartListPage.waitForTableLoad();
 
-  // Verify chart is visible in list
-  await expect(chartListPage.getChartRow(chartName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created chart appears.
+  await expect(chartListPage.getChartRow(chartName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Set up API response intercept for export endpoint
   const exportResponsePromise = waitForGet(page, ENDPOINTS.CHART_EXPORT);
@@ -202,9 +212,14 @@ test('should bulk delete multiple charts', async ({
   await chartListPage.goto();
   await chartListPage.waitForTableLoad();
 
-  // Verify both charts are visible in list
-  await expect(chartListPage.getChartRow(chart1.name)).toBeVisible();
-  await expect(chartListPage.getChartRow(chart2.name)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created charts appear.
+  await expect(chartListPage.getChartRow(chart1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(chartListPage.getChartRow(chart2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Enable bulk select mode
   await chartListPage.clickBulkSelectButton();
@@ -229,13 +244,13 @@ test('should bulk delete multiple charts', async ({
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
-  // Verify both charts are removed from list
-  await expect(chartListPage.getChartRow(chart1.name)).not.toBeVisible();
-  await expect(chartListPage.getChartRow(chart2.name)).not.toBeVisible();
+  // Verify both charts are removed from list (deleted rows leave the DOM)
+  await expect(chartListPage.getChartRow(chart1.name)).toHaveCount(0);
+  await expect(chartListPage.getChartRow(chart2.name)).toHaveCount(0);
 
   // Backend verification: Both return 404
   for (const chart of [chart1, chart2]) {
@@ -259,8 +274,11 @@ test('should edit chart name from card view', async ({ page, testAssets }) => {
   await cardListPage.gotoCardView();
   await cardListPage.waitForCardLoad();
 
-  // Verify chart card is visible
-  await expect(cardListPage.getChartCard(chartName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created chart card appears.
+  await expect(cardListPage.getChartCard(chartName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Open card dropdown and click edit
   await cardListPage.clickCardEditAction(chartName);
@@ -285,13 +303,16 @@ test('should edit chart name from card view', async ({ page, testAssets }) => {
   // Modal should close
   await propertiesModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
   // Verify the renamed card appears in card view and old name is gone
-  await expect(cardListPage.getChartCard(newName)).toBeVisible();
-  await expect(cardListPage.getChartCard(chartName)).not.toBeVisible();
+  // (the old card name is removed from the DOM after the rename re-render).
+  await expect(cardListPage.getChartCard(newName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(cardListPage.getChartCard(chartName)).toHaveCount(0);
 
   // Backend verification: API returns updated name
   const response = await apiGetChart(page, chartId);
@@ -304,6 +325,11 @@ test('should bulk export multiple charts', async ({
   chartListPage,
   testAssets,
 }) => {
+  // Chains create×2 → refresh → bulk select → export. Matches the
+  // sibling bulk-delete test's budget so the export response wait below
+  // can exceed the 30s default without hitting the test timeout.
+  test.setTimeout(TIMEOUT.SLOW_TEST);
+
   // Create 2 throwaway charts for bulk export
   const [chart1, chart2] = await Promise.all([
     createTestChart(page, testAssets, test.info(), {
@@ -318,9 +344,14 @@ test('should bulk export multiple charts', async ({
   await chartListPage.goto();
   await chartListPage.waitForTableLoad();
 
-  // Verify both charts are visible in list
-  await expect(chartListPage.getChartRow(chart1.name)).toBeVisible();
-  await expect(chartListPage.getChartRow(chart2.name)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created charts appear.
+  await expect(chartListPage.getChartRow(chart1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(chartListPage.getChartRow(chart2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Enable bulk select mode
   await chartListPage.clickBulkSelectButton();
@@ -329,8 +360,12 @@ test('should bulk export multiple charts', async ({
   await chartListPage.selectChartCheckbox(chart1.name);
   await chartListPage.selectChartCheckbox(chart2.name);
 
-  // Set up API response intercept for export endpoint
-  const exportResponsePromise = waitForGet(page, ENDPOINTS.CHART_EXPORT);
+  // Set up API response intercept BEFORE the click that triggers it.
+  // Exports of multiple charts can take longer than 30s under load,
+  // so use SLOW_TEST instead of the default test-timeout-bound budget.
+  const exportResponsePromise = waitForGet(page, ENDPOINTS.CHART_EXPORT, {
+    timeout: TIMEOUT.SLOW_TEST,
+  });
 
   // Click bulk export action
   await chartListPage.clickBulkAction('Export');

superset-frontend/playwright/tests/dashboard/dashboard-list.spec.ts

@@ -68,33 +68,34 @@ test('should delete a dashboard with confirmation', async ({
   await dashboardListPage.goto();
   await dashboardListPage.waitForTableLoad();
 
-  // Verify dashboard is visible in list
-  await expect(dashboardListPage.getDashboardRow(dashboardName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dashboard appears.
+  await expect(dashboardListPage.getDashboardRow(dashboardName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Click delete action button
   await dashboardListPage.clickDeleteAction(dashboardName);
 
   // Delete confirmation modal should appear
   const deleteModal = new DeleteConfirmationModal(page);
-  await deleteModal.waitForVisible();
+  await deleteModal.waitForReady();
 
   // Type "DELETE" to confirm
   await deleteModal.fillConfirmationInput('DELETE');
 
-  // Click the Delete button
+  // Click the Delete button (waits for it to become enabled)
   await deleteModal.clickDelete();
 
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
   // Verify dashboard is removed from list
-  await expect(
-    dashboardListPage.getDashboardRow(dashboardName),
-  ).not.toBeVisible();
+  await expect(dashboardListPage.getDashboardRow(dashboardName)).toHaveCount(0);
 
   // Backend verification: API returns 404
   await expectDeleted(page, ENDPOINTS.DASHBOARD, dashboardId, {
@@ -119,8 +120,11 @@ test('should export a dashboard as a zip file', async ({
   await dashboardListPage.goto();
   await dashboardListPage.waitForTableLoad();
 
-  // Verify dashboard is visible in list
-  await expect(dashboardListPage.getDashboardRow(dashboardName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dashboard appears.
+  await expect(dashboardListPage.getDashboardRow(dashboardName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Set up API response intercept for export endpoint
   const exportResponsePromise = waitForGet(page, ENDPOINTS.DASHBOARD_EXPORT);
@@ -157,13 +161,14 @@ test('should bulk delete multiple dashboards', async ({
   await dashboardListPage.goto();
   await dashboardListPage.waitForTableLoad();
 
-  // Verify both dashboards are visible in list
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard1.name),
-  ).toBeVisible();
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard2.name),
-  ).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dashboards appear.
+  await expect(dashboardListPage.getDashboardRow(dashboard1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(dashboardListPage.getDashboardRow(dashboard2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Enable bulk select mode
   await dashboardListPage.clickBulkSelectButton();
@@ -188,17 +193,17 @@ test('should bulk delete multiple dashboards', async ({
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
-  // Verify both dashboards are removed from list
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard1.name),
-  ).not.toBeVisible();
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard2.name),
-  ).not.toBeVisible();
+  // Verify both dashboards are removed from list (deleted rows leave the DOM)
+  await expect(dashboardListPage.getDashboardRow(dashboard1.name)).toHaveCount(
+    0,
+  );
+  await expect(dashboardListPage.getDashboardRow(dashboard2.name)).toHaveCount(
+    0,
+  );
 
   // Backend verification: Both return 404
   for (const dashboard of [dashboard1, dashboard2]) {
@@ -213,6 +218,11 @@ test('should bulk export multiple dashboards', async ({
   dashboardListPage,
   testAssets,
 }) => {
+  // Chains create×2 → refresh → bulk select → export. Matches the
+  // sibling bulk-delete test's budget so the export response wait below
+  // can exceed the 30s default without hitting the test timeout.
+  test.setTimeout(TIMEOUT.SLOW_TEST);
+
   // Create 2 throwaway dashboards for bulk export
   const [dashboard1, dashboard2] = await Promise.all([
     createTestDashboard(page, testAssets, test.info(), {
@@ -227,25 +237,30 @@ test('should bulk export multiple dashboards', async ({
   await dashboardListPage.goto();
   await dashboardListPage.waitForTableLoad();
 
-  // Verify both dashboards are visible in list
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard1.name),
-  ).toBeVisible();
-  await expect(
-    dashboardListPage.getDashboardRow(dashboard2.name),
-  ).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dashboards appear.
+  await expect(dashboardListPage.getDashboardRow(dashboard1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(dashboardListPage.getDashboardRow(dashboard2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
-  // Enable bulk select mode
+  // Enable bulk select mode (waits for the checkbox column to render)
   await dashboardListPage.clickBulkSelectButton();
 
-  // Select both dashboards
+  // Select both dashboards (each call asserts the checkbox is checked)
   await dashboardListPage.selectDashboardCheckbox(dashboard1.name);
   await dashboardListPage.selectDashboardCheckbox(dashboard2.name);
 
-  // Set up API response intercept for export endpoint
-  const exportResponsePromise = waitForGet(page, ENDPOINTS.DASHBOARD_EXPORT);
+  // Set up API response intercept BEFORE the click that triggers it.
+  // Exports of multiple dashboards can take longer than 30s under load,
+  // so use SLOW_TEST instead of the default test-timeout-bound budget.
+  const exportResponsePromise = waitForGet(page, ENDPOINTS.DASHBOARD_EXPORT, {
+    timeout: TIMEOUT.SLOW_TEST,
+  });
 
-  // Click bulk export action
+  // Click bulk export action (waits for the action button to render)
   await dashboardListPage.clickBulkAction('Export');
 
   // Wait for export API response and validate zip contains both dashboards
@@ -293,12 +308,12 @@ test.describe('import dashboard', () => {
       label: `Dashboard ${dashboardId}`,
     });
 
-    // Refresh to confirm dashboard is no longer in the list
+    // Refresh to confirm dashboard is no longer in the list (deleted rows leave the DOM)
     await dashboardListPage.goto();
     await dashboardListPage.waitForTableLoad();
-    await expect(
-      dashboardListPage.getDashboardRow(dashboardName),
-    ).not.toBeVisible();
+    await expect(dashboardListPage.getDashboardRow(dashboardName)).toHaveCount(
+      0,
+    );
 
     // Click the import button
     await dashboardListPage.clickImportButton();
@@ -350,7 +365,7 @@ test.describe('import dashboard', () => {
     // Modal should close on success
     await importModal.waitForHidden({ timeout: TIMEOUT.FILE_IMPORT });
 
-    // Verify success toast appears
+    // Verify success toast appears.
     const toast = new Toast(page);
     await expect(toast.getSuccess()).toBeVisible({ timeout: 10000 });
 
@@ -358,10 +373,11 @@ test.describe('import dashboard', () => {
     await dashboardListPage.goto();
     await dashboardListPage.waitForTableLoad();
 
-    // Verify dashboard appears in list
-    await expect(
-      dashboardListPage.getDashboardRow(dashboardName),
-    ).toBeVisible();
+    // The list query is asynchronous; allow extra time on slow CI before the
+    // freshly-imported dashboard appears.
+    await expect(dashboardListPage.getDashboardRow(dashboardName)).toBeVisible({
+      timeout: TIMEOUT.API_RESPONSE,
+    });
 
     // Track for cleanup: look up the reimported dashboard by title
     const reimported = await getDashboardByName(page, dashboardName);

superset-frontend/playwright/tests/dataset/dataset-list.spec.ts

@@ -107,8 +107,11 @@ test('should delete a dataset with confirmation', async ({
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
 
-  // Verify dataset is visible in list
-  await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dataset appears.
+  await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Click delete action button
   await datasetListPage.clickDeleteAction(datasetName);
@@ -126,14 +129,13 @@ test('should delete a dataset with confirmation', async ({
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears with correct message
+  // Verify success toast appears with correct message.
   const toast = new Toast(page);
-  const successToast = toast.getSuccess();
-  await expect(successToast).toBeVisible();
+  await expect(toast.getSuccess()).toBeVisible();
   await expect(toast.getMessage()).toContainText('Deleted');
 
-  // Verify dataset is removed from list
-  await expect(datasetListPage.getDatasetRow(datasetName)).not.toBeVisible();
+  // Verify dataset is removed from list (deleted rows leave the DOM)
+  await expect(datasetListPage.getDatasetRow(datasetName)).toHaveCount(0);
 
   // Verify via API that dataset no longer exists (404)
   await expectDeleted(page, ENDPOINTS.DATASET, datasetId, {
@@ -155,10 +157,13 @@ test('should duplicate a dataset with new name', async ({
   );
   const duplicateName = `duplicate_${Date.now()}_${test.info().parallelIndex}`;
 
-  // Navigate to list and verify original dataset is visible
+  // Navigate to list and verify original dataset is visible.
+  // The list query is asynchronous; allow extra time on slow CI.
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
-  await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible();
+  await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Set up response intercept to capture duplicate dataset ID
   const duplicateResponsePromise = waitForPost(
@@ -201,9 +206,14 @@ test('should duplicate a dataset with new name', async ({
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
 
-  // Verify both datasets exist in list
-  await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible();
-  await expect(datasetListPage.getDatasetRow(duplicateName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // duplicate appears alongside the original.
+  await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(datasetListPage.getDatasetRow(duplicateName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // API Verification: Fetch both datasets via detail API for consistent comparison
   // (list API may return undefined for fields that detail API returns as null)
@@ -256,6 +266,11 @@ test('should export multiple datasets via bulk select action', async ({
   datasetListPage,
   testAssets,
 }) => {
+  // Chains create×2 → refresh → bulk select → export. Matches the
+  // sibling bulk-delete test's budget so the export response wait below
+  // can exceed the 30s default without hitting the test timeout.
+  test.setTimeout(TIMEOUT.SLOW_TEST);
+
   // Create 2 throwaway datasets for bulk export
   const [dataset1, dataset2] = await Promise.all([
     createTestDataset(page, testAssets, test.info(), {
@@ -270,9 +285,14 @@ test('should export multiple datasets via bulk select action', async ({
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
 
-  // Verify both datasets are visible in list
-  await expect(datasetListPage.getDatasetRow(dataset1.name)).toBeVisible();
-  await expect(datasetListPage.getDatasetRow(dataset2.name)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created datasets appear.
+  await expect(datasetListPage.getDatasetRow(dataset1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(datasetListPage.getDatasetRow(dataset2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Enable bulk select mode
   await datasetListPage.clickBulkSelectButton();
@@ -281,8 +301,12 @@ test('should export multiple datasets via bulk select action', async ({
   await datasetListPage.selectDatasetCheckbox(dataset1.name);
   await datasetListPage.selectDatasetCheckbox(dataset2.name);
 
-  // Set up API response intercept for export endpoint
-  const exportResponsePromise = waitForGet(page, ENDPOINTS.DATASET_EXPORT);
+  // Set up API response intercept BEFORE the click that triggers it.
+  // Exports of multiple datasets can take longer than 30s under load,
+  // so use SLOW_TEST instead of the default test-timeout-bound budget.
+  const exportResponsePromise = waitForGet(page, ENDPOINTS.DATASET_EXPORT, {
+    timeout: TIMEOUT.SLOW_TEST,
+  });
 
   // Click bulk export action
   await datasetListPage.clickBulkAction('Export');
@@ -312,8 +336,11 @@ test('should edit dataset name via modal', async ({
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
 
-  // Verify dataset is visible in list
-  await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created dataset appears.
+  await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Click edit action to open modal
   await datasetListPage.clickEditAction(datasetName);
@@ -348,7 +375,7 @@ test('should edit dataset name via modal', async ({
   // Modal should close
   await editModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible({ timeout: 10000 });
 
@@ -377,9 +404,14 @@ test('should bulk delete multiple datasets', async ({
   await datasetListPage.goto();
   await datasetListPage.waitForTableLoad();
 
-  // Verify both datasets are visible in list
-  await expect(datasetListPage.getDatasetRow(dataset1.name)).toBeVisible();
-  await expect(datasetListPage.getDatasetRow(dataset2.name)).toBeVisible();
+  // The list query is asynchronous; allow extra time on slow CI before the
+  // freshly-created datasets appear.
+  await expect(datasetListPage.getDatasetRow(dataset1.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
+  await expect(datasetListPage.getDatasetRow(dataset2.name)).toBeVisible({
+    timeout: TIMEOUT.API_RESPONSE,
+  });
 
   // Enable bulk select mode
   await datasetListPage.clickBulkSelectButton();
@@ -404,13 +436,13 @@ test('should bulk delete multiple datasets', async ({
   // Modal should close
   await deleteModal.waitForHidden();
 
-  // Verify success toast appears
+  // Verify success toast appears.
   const toast = new Toast(page);
   await expect(toast.getSuccess()).toBeVisible();
 
-  // Verify both datasets are removed from list
-  await expect(datasetListPage.getDatasetRow(dataset1.name)).not.toBeVisible();
-  await expect(datasetListPage.getDatasetRow(dataset2.name)).not.toBeVisible();
+  // Verify both datasets are removed from list (deleted rows leave the DOM)
+  await expect(datasetListPage.getDatasetRow(dataset1.name)).toHaveCount(0);
+  await expect(datasetListPage.getDatasetRow(dataset2.name)).toHaveCount(0);
 
   // Verify via API that datasets no longer exist (404)
   await expectDeleted(page, ENDPOINTS.DATASET, dataset1.id, {
@@ -455,10 +487,10 @@ test.describe('import dataset', () => {
       label: `Dataset ${datasetId}`,
     });
 
-    // Refresh to confirm dataset is no longer in the list
+    // Refresh to confirm dataset is no longer in the list (deleted rows leave the DOM)
     await datasetListPage.goto();
     await datasetListPage.waitForTableLoad();
-    await expect(datasetListPage.getDatasetRow(datasetName)).not.toBeVisible();
+    await expect(datasetListPage.getDatasetRow(datasetName)).toHaveCount(0);
 
     // Click the import button
     await datasetListPage.clickImportButton();
@@ -507,7 +539,7 @@ test.describe('import dataset', () => {
     // Modal should close on success
     await importModal.waitForHidden({ timeout: TIMEOUT.FILE_IMPORT });
 
-    // Verify success toast appears
+    // Verify success toast appears.
     const toast = new Toast(page);
     await expect(toast.getSuccess()).toBeVisible({ timeout: 10000 });
 
@@ -515,8 +547,11 @@ test.describe('import dataset', () => {
     await datasetListPage.goto();
     await datasetListPage.waitForTableLoad();
 
-    // Verify dataset appears in list
-    await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible();
+    // The list query is asynchronous; allow extra time on slow CI before the
+    // freshly-imported dataset appears.
+    await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible({
+      timeout: TIMEOUT.API_RESPONSE,
+    });
 
     // Track for cleanup: the dataset import API returns {"message": "OK"}
     // with no ID, so look up the reimported dataset by name.

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/htmlTextFilterValueGetter.test.ts

@@ -1,83 +0,0 @@
-/**
- * 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 { ValueGetterParams } from '@superset-ui/core/components/ThemedAgGridReact';
-import htmlTextFilterValueGetter, {
-  htmlTextComparator,
-} from './htmlTextFilterValueGetter';
-
-const makeParams = (value: unknown): ValueGetterParams =>
-  ({
-    data: { foo: value },
-    colDef: { field: 'foo' },
-  }) as unknown as ValueGetterParams;
-
-test('htmlTextFilterValueGetter extracts visible text from HTML anchor', () => {
-  expect(
-    htmlTextFilterValueGetter(
-      makeParams(
-        '<a href="https://jira.example.com/123/S18_3232">S18_3232</a>',
-      ),
-    ),
-  ).toBe('S18_3232');
-});
-
-test('htmlTextFilterValueGetter strips nested HTML markup', () => {
-  expect(
-    htmlTextFilterValueGetter(
-      makeParams('<div><strong>Hello</strong> <em>World</em></div>'),
-    ),
-  ).toBe('Hello World');
-});
-
-test('htmlTextFilterValueGetter passes plain strings through', () => {
-  expect(htmlTextFilterValueGetter(makeParams('plain value'))).toBe(
-    'plain value',
-  );
-});
-
-test('htmlTextFilterValueGetter passes non-string values through', () => {
-  expect(htmlTextFilterValueGetter(makeParams(42))).toBe(42);
-  expect(htmlTextFilterValueGetter(makeParams(null))).toBeNull();
-  expect(htmlTextFilterValueGetter(makeParams(undefined))).toBeUndefined();
-});
-
-test('htmlTextComparator orders by visible text, not raw HTML', () => {
-  // URL prefixes (zzz vs bbb) would flip the order under raw-HTML sort,
-  // but the visible labels (S700_4002 vs S72_3212) sort the other way.
-  const left = '<a href="https://jira.example.com/zzz/S700_4002">S700_4002</a>';
-  const right = '<a href="https://jira.example.com/bbb/S72_3212">S72_3212</a>';
-  expect(htmlTextComparator(left, right)).toBeLessThan(0);
-});
-
-test('htmlTextComparator handles nulls and numbers', () => {
-  expect(htmlTextComparator(null, null)).toBe(0);
-  expect(htmlTextComparator(null, 'x')).toBeLessThan(0);
-  expect(htmlTextComparator('x', null)).toBeGreaterThan(0);
-  expect(htmlTextComparator(1, 2)).toBeLessThan(0);
-  expect(htmlTextComparator(2, 1)).toBeGreaterThan(0);
-});
-
-test('htmlTextComparator preserves default codepoint ordering for plain strings', () => {
-  // AG Grid's default string comparator orders by codepoint, so 'Z' (90)
-  // sorts before 'a' (97). A locale-aware comparator would flip this —
-  // verify we match the default so plain string columns are unaffected.
-  expect(htmlTextComparator('Z', 'a')).toBeLessThan(0);
-  expect(htmlTextComparator('a', 'Z')).toBeGreaterThan(0);
-  expect(htmlTextComparator('apple', 'banana')).toBeLessThan(0);
-});

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/htmlTextFilterValueGetter.ts

@@ -1,74 +0,0 @@
-/**
- * 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 { isProbablyHTML, sanitizeHtml } from '@superset-ui/core';
-import { ValueGetterParams } from '@superset-ui/core/components/ThemedAgGridReact';
-
-const stripHtmlToText = (html: string): string => {
-  const doc = new DOMParser().parseFromString(sanitizeHtml(html), 'text/html');
-  return (doc.body.textContent || '').trim();
-};
-
-// Cache the comparator-ready form per raw string. Both the HTML-detection
-// step (`isProbablyHTML`, which itself invokes DOMParser for HTML-looking
-// values) and the extraction step (`stripHtmlToText`, also DOMParser) are
-// expensive; sort runs `O(n log n)` comparator calls against the same set
-// of cell values. Memoizing the combined detection + extraction means each
-// unique cell value pays the cost once per session. Module-level scope;
-// bounded by the count of unique string cell values seen.
-const comparableTextCache = new Map<string, string>();
-
-const toComparableText = (raw: string): string => {
-  const cached = comparableTextCache.get(raw);
-  if (cached !== undefined) return cached;
-  const normalized = isProbablyHTML(raw) ? stripHtmlToText(raw) : raw;
-  comparableTextCache.set(raw, normalized);
-  return normalized;
-};
-
-/**
- * Returns the visible-text representation of an HTML cell value so AG Grid
- * filters and sort operate on what the user sees, not the underlying markup.
- * Pass-through for non-HTML values.
- */
-const htmlTextFilterValueGetter = (params: ValueGetterParams) => {
-  const raw = params.data?.[params.colDef.field as string];
-  return typeof raw === 'string' ? toComparableText(raw) : raw;
-};
-
-/**
- * Comparator that mirrors AG Grid's default string comparator (codepoint
- * order, nulls first), but extracts visible text from HTML values first
- * so HTML cells sort by their displayed label. Plain (non-HTML) values
- * pass through unchanged, preserving default ordering — e.g. 'Z' still
- * sorts before 'a' as it does under the default comparator.
- */
-export const htmlTextComparator = (a: unknown, b: unknown): number => {
-  const toText = (v: unknown) =>
-    typeof v === 'string' ? toComparableText(v) : v;
-  const aT = toText(a);
-  const bT = toText(b);
-  if (aT == null && bT == null) return 0;
-  if (aT == null) return -1;
-  if (bT == null) return 1;
-  if (typeof aT === 'number' && typeof bT === 'number') return aT - bT;
-  if (aT === bT) return 0;
-  return aT < bT ? -1 : 1;
-};
-
-export default htmlTextFilterValueGetter;

superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts

@@ -32,9 +32,6 @@ import {
 } from '../types';
 import getCellClass from './getCellClass';
 import filterValueGetter from './filterValueGetter';
-import htmlTextFilterValueGetter, {
-  htmlTextComparator,
-} from './htmlTextFilterValueGetter';
 import dateFilterComparator from './dateFilterComparator';
 import DateWithFormatter from './DateWithFormatter';
 import { getAggFunc } from './getAggFunc';
@@ -320,24 +317,6 @@ export const useColDefs = ({
         ...(isPercentMetric && {
           filterValueGetter,
         }),
-        ...(dataType === GenericDataType.String &&
-          !serverPagination && {
-            // HTML cells (e.g. anchor markup) are rendered by TextCellRenderer
-            // via dangerouslySetInnerHTML; without these the filter and sort
-            // operate on raw HTML so the URL inside the markup dictates order
-            // and the "Contains" filter matches against the raw HTML string.
-            //
-            // Gated on !serverPagination: in server-pagination mode sort and
-            // filter are both delegated to the backend (which sees raw HTML
-            // in the database), so applying the visible-text getter only on
-            // the client would create a mismatch where the typed filter
-            // value is stripped client-side but the server query still
-            // operates on the raw HTML. Server-paginated tables with HTML
-            // columns are out of scope for this fix and would require
-            // server-side handling.
-            filterValueGetter: htmlTextFilterValueGetter,
-            comparator: htmlTextComparator,
-          }),
         ...(dataType === GenericDataType.Temporal && {
           // Use dateFilterValueGetter so AG Grid correctly identifies null dates for blank filter
           filterValueGetter: dateFilterValueGetter,

superset-frontend/src/components/Chart/Chart.tsx

@@ -16,9 +16,9 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { ErrorInfo, useCallback, useEffect, useRef } from 'react';
-import { t } from '@apache-superset/core/translation';
+import { ErrorInfo, PureComponent } from 'react';
 import { logging } from '@apache-superset/core/utils';
+import { t } from '@apache-superset/core/translation';
 import {
   ensureIsArray,
   FeatureFlag,
@@ -60,7 +60,7 @@ export interface ChartProps {
   sharedLabelColors?: string;
   width: number;
   height: number;
-  setControlValue?: (name: string, value: unknown) => void;
+  setControlValue: (name: string, value: unknown) => void;
   timeout?: number;
   vizType: string;
   triggerRender?: boolean;
@@ -69,7 +69,7 @@ export interface ChartProps {
   chartAlert?: string;
   chartStatus?: ChartStatus;
   chartStackTrace?: string;
-  queriesResponse?: ChartState['queriesResponse'];
+  queriesResponse: ChartState['queriesResponse'];
   latestQueryFormData?: ChartState['latestQueryFormData'];
   triggerQuery?: boolean;
   chartIsStale?: boolean;
@@ -126,6 +126,19 @@ const NONEXISTENT_DATASET = t(
   'The dataset associated with this chart no longer exists',
 );
 
+const defaultProps: Partial<ChartProps> = {
+  addFilter: () => BLANK,
+  onFilterMenuOpen: () => BLANK,
+  onFilterMenuClose: () => BLANK,
+  initialValues: BLANK,
+  setControlValue: () => BLANK,
+  triggerRender: false,
+  dashboardId: undefined,
+  chartStackTrace: undefined,
+  force: false,
+  isInView: true,
+};
+
 const Styles = styled.div<{ height: number; width?: number }>`
   min-height: ${p => p.height}px;
   position: relative;
@@ -173,321 +186,252 @@ const MessageSpan = styled.span`
   color: ${({ theme }) => theme.colorText};
 `;
 
-function Chart({
-  addFilter = () => BLANK,
-  onFilterMenuOpen = () => BLANK,
-  onFilterMenuClose = () => BLANK,
-  initialValues = BLANK,
-  setControlValue = () => BLANK,
-  triggerRender = false,
-  dashboardId,
-  chartStackTrace,
-  force = false,
-  isInView = true,
-  ...restProps
-}: ChartProps): JSX.Element {
-  const {
-    actions,
-    chartId,
-    datasource,
-    formData,
-    timeout,
-    ownState,
-    chartAlert,
-    chartStatus,
-    queriesResponse = [],
-    errorMessage,
-    chartIsStale,
-    width,
-    height,
-    datasetsStatus,
-    onQuery,
-    annotationData,
-    vizType,
-    latestQueryFormData,
-    triggerQuery,
-    postTransformProps,
-    emitCrossFilters,
-    onChartStateChange,
-    suppressLoadingSpinner,
-    filterState,
-  } = restProps;
+class Chart extends PureComponent<ChartProps, {}> {
+  static defaultProps = defaultProps;
 
-  const renderStartTimeRef = useRef<number>(Logger.getTimestamp());
-  // Update on each render to accurately track render duration
-  renderStartTimeRef.current = Logger.getTimestamp();
+  renderStartTime: number;
 
-  const shouldRenderChart = useCallback(
-    () =>
-      isInView ||
+  constructor(props: ChartProps) {
+    super(props);
+    this.renderStartTime = Logger.getTimestamp();
+    this.handleRenderContainerFailure =
+      this.handleRenderContainerFailure.bind(this);
+  }
+
+  componentDidMount() {
+    if (this.props.triggerQuery) {
+      this.runQuery();
+    }
+  }
+
+  componentDidUpdate() {
+    if (this.props.triggerQuery) {
+      this.runQuery();
+    }
+  }
+
+  shouldRenderChart() {
+    return (
+      this.props.isInView ||
       !isFeatureEnabled(FeatureFlag.DashboardVirtualization) ||
-      isCurrentUserBot(),
-    [isInView],
-  );
+      isCurrentUserBot()
+    );
+  }
 
-  const runQuery = useCallback(() => {
+  runQuery() {
     if (
       isFeatureEnabled(FeatureFlag.DashboardVirtualizationDeferData) &&
-      !shouldRenderChart()
+      !this.shouldRenderChart()
     ) {
       return;
     }
     // Create chart with POST request
-    actions.postChartFormData(
-      formData,
-      Boolean(force || getUrlParam(URL_PARAMS.force)), // allow override via url params force=true
-      timeout,
-      chartId,
-      dashboardId,
-      ownState,
+    this.props.actions.postChartFormData(
+      this.props.formData,
+      Boolean(this.props.force || getUrlParam(URL_PARAMS.force)), // allow override via url params force=true
+      this.props.timeout,
+      this.props.chartId,
+      this.props.dashboardId,
+      this.props.ownState,
     );
-  }, [
-    actions,
-    chartId,
-    dashboardId,
-    formData,
-    force,
-    ownState,
-    shouldRenderChart,
-    timeout,
-  ]);
+  }
 
-  const handleRenderContainerFailure = useCallback(
-    (error: Error, info: ErrorInfo) => {
-      logging.warn(error);
-      actions.chartRenderingFailed(
-        error.toString(),
-        chartId,
-        info?.componentStack ?? null,
-      );
-
-      actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
-        slice_id: chartId,
-        has_err: true,
-        error_details: error.toString(),
-        start_offset: renderStartTimeRef.current,
-        ts: new Date().getTime(),
-        duration: Logger.getTimestamp() - renderStartTimeRef.current,
-      });
-    },
-    [actions, chartId],
-  );
-
-  // componentDidMount and componentDidUpdate combined
-  useEffect(() => {
-    if (triggerQuery) {
-      runQuery();
-    }
-  }, [triggerQuery, runQuery]);
-
-  const renderErrorMessage = useCallback(
-    (queryResponse: ChartErrorType) => {
-      const error = queryResponse?.errors?.[0];
-      const message = chartAlert || queryResponse?.message;
-
-      // if datasource is still loading, don't render JS errors
-      // but always show backend API errors (which have an errors array)
-      // so users can see real issues like auth failures
-      if (
-        !error &&
-        chartAlert !== undefined &&
-        chartAlert !== NONEXISTENT_DATASET &&
-        datasource === PLACEHOLDER_DATASOURCE &&
-        datasetsStatus !== ResourceStatus.Error
-      ) {
-        return (
-          <Styles
-            key={chartId}
-            data-ui-anchor="chart"
-            className="chart-container"
-            data-test="chart-container"
-            height={height}
-          >
-            <Loading size={dashboardId ? 's' : 'm'} muted={!!dashboardId} />
-          </Styles>
-        );
-      }
-
-      return (
-        <ChartErrorMessage
-          key={chartId}
-          chartId={chartId}
-          error={error}
-          subtitle={message}
-          link={queryResponse ? queryResponse.link : undefined}
-          source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
-          stackTrace={chartStackTrace}
-        />
-      );
-    },
-    [
-      chartAlert,
+  handleRenderContainerFailure(error: Error, info: ErrorInfo) {
+    const { actions, chartId } = this.props;
+    logging.warn(error);
+    actions.chartRenderingFailed(
+      error.toString(),
       chartId,
+      info?.componentStack ?? null,
+    );
+
+    actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+      slice_id: chartId,
+      has_err: true,
+      error_details: error.toString(),
+      start_offset: this.renderStartTime,
+      ts: new Date().getTime(),
+      duration: Logger.getTimestamp() - this.renderStartTime,
+    });
+  }
+
+  renderErrorMessage(queryResponse: ChartErrorType) {
+    const {
+      chartId,
+      chartAlert,
       chartStackTrace,
-      dashboardId,
-      datasetsStatus,
       datasource,
+      dashboardId,
       height,
-    ],
-  );
-
-  const renderSpinner = useCallback(
-    (databaseName: string | undefined) => {
-      const message = databaseName
-        ? t('Waiting on %s', databaseName)
-        : t('Waiting on database...');
+      datasetsStatus,
+    } = this.props;
+    const error = queryResponse?.errors?.[0];
+    const message = chartAlert || queryResponse?.message;
 
+    // if datasource is still loading, don't render JS errors
+    // but always show backend API errors (which have an errors array)
+    // so users can see real issues like auth failures
+    if (
+      !error &&
+      chartAlert !== undefined &&
+      chartAlert !== NONEXISTENT_DATASET &&
+      datasource === PLACEHOLDER_DATASOURCE &&
+      datasetsStatus !== ResourceStatus.Error
+    ) {
       return (
-        <LoadingDiv>
+        <Styles
+          key={chartId}
+          data-ui-anchor="chart"
+          className="chart-container"
+          data-test="chart-container"
+          height={height}
+        >
           <Loading
-            position="inline-centered"
-            size={dashboardId ? 's' : 'm'}
-            muted={!!dashboardId}
+            size={this.props.dashboardId ? 's' : 'm'}
+            muted={!!this.props.dashboardId}
           />
-          <MessageSpan>{message}</MessageSpan>
-        </LoadingDiv>
+        </Styles>
       );
-    },
-    [dashboardId],
-  );
+    }
 
-  const renderChartContainer = useCallback(
-    () => (
+    return (
+      <ChartErrorMessage
+        key={chartId}
+        chartId={chartId}
+        error={error}
+        subtitle={message}
+        link={queryResponse ? queryResponse.link : undefined}
+        source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+        stackTrace={chartStackTrace}
+      />
+    );
+  }
+
+  renderSpinner(databaseName: string | undefined) {
+    const message = databaseName
+      ? t('Waiting on %s', databaseName)
+      : t('Waiting on database...');
+
+    return (
+      <LoadingDiv>
+        <Loading
+          position="inline-centered"
+          size={this.props.dashboardId ? 's' : 'm'}
+          muted={!!this.props.dashboardId}
+        />
+        <MessageSpan>{message}</MessageSpan>
+      </LoadingDiv>
+    );
+  }
+
+  renderChartContainer() {
+    return (
       <div className="slice_container" data-test="slice-container">
-        {shouldRenderChart() ? (
+        {this.shouldRenderChart() ? (
           <ChartRenderer
-            annotationData={annotationData}
-            actions={actions}
-            chartId={chartId}
-            datasource={datasource}
-            initialValues={initialValues}
-            formData={formData}
-            height={height}
-            width={width}
-            setControlValue={setControlValue}
-            vizType={vizType}
-            triggerRender={triggerRender}
-            chartAlert={chartAlert}
-            chartStatus={chartStatus}
-            queriesResponse={queriesResponse}
-            triggerQuery={triggerQuery}
-            chartIsStale={chartIsStale}
-            addFilter={addFilter}
-            onFilterMenuOpen={onFilterMenuOpen}
-            onFilterMenuClose={onFilterMenuClose}
-            ownState={ownState}
-            postTransformProps={postTransformProps}
-            emitCrossFilters={emitCrossFilters}
-            onChartStateChange={onChartStateChange}
-            latestQueryFormData={latestQueryFormData}
-            filterState={filterState}
-            suppressLoadingSpinner={suppressLoadingSpinner}
-            source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+            {...this.props}
+            source={
+              this.props.dashboardId
+                ? ChartSource.Dashboard
+                : ChartSource.Explore
+            }
+            data-test={this.props.vizType}
           />
         ) : (
-          <Loading size={dashboardId ? 's' : 'm'} muted={!!dashboardId} />
+          <Loading
+            size={this.props.dashboardId ? 's' : 'm'}
+            muted={!!this.props.dashboardId}
+          />
         )}
       </div>
-    ),
-    [
-      actions,
-      addFilter,
-      annotationData,
-      chartAlert,
-      chartId,
-      chartIsStale,
-      chartStatus,
-      dashboardId,
-      datasource,
-      emitCrossFilters,
-      filterState,
-      formData,
+    );
+  }
+
+  render() {
+    const {
       height,
-      initialValues,
-      latestQueryFormData,
-      onChartStateChange,
-      onFilterMenuClose,
-      onFilterMenuOpen,
-      ownState,
-      postTransformProps,
-      queriesResponse,
-      setControlValue,
-      shouldRenderChart,
-      suppressLoadingSpinner,
-      triggerQuery,
-      triggerRender,
-      vizType,
+      chartAlert,
+      chartStatus,
+      datasource,
+      errorMessage,
+      chartIsStale,
+      queriesResponse = [],
       width,
-    ],
-  );
+    } = this.props;
 
-  const databaseName =
-    datasource?.parent?.name ??
-    (datasource?.database?.name as string | undefined);
+    const databaseName =
+      datasource?.parent?.name ??
+      (datasource?.database?.name as string | undefined);
 
-  const isLoading = chartStatus === 'loading';
-  // Suppress spinner during auto-refresh to avoid visual flicker
-  const showSpinner = isLoading && !suppressLoadingSpinner;
+    const isLoading = chartStatus === 'loading';
+    // Suppress spinner during auto-refresh to avoid visual flicker
+    const showSpinner = isLoading && !this.props.suppressLoadingSpinner;
 
-  if (chartStatus === 'failed') {
-    return (
-      <ErrorContainer height={height}>
-        {queriesResponse?.map(item =>
-          renderErrorMessage(item as ChartErrorType),
-        )}
-      </ErrorContainer>
-    );
-  }
+    if (chartStatus === 'failed') {
+      return (
+        <ErrorContainer height={height}>
+          {queriesResponse?.map(item =>
+            this.renderErrorMessage(item as ChartErrorType),
+          )}
+        </ErrorContainer>
+      );
+    }
 
-  if (errorMessage && ensureIsArray(queriesResponse).length === 0) {
-    return (
-      <EmptyState
-        size="large"
-        title={t('Add required control values to preview chart')}
-        description={getChartRequiredFieldsMissingMessage(true)}
-        image="chart.svg"
-      />
-    );
-  }
-  if (
-    !isLoading &&
-    !chartAlert &&
-    !errorMessage &&
-    chartIsStale &&
-    ensureIsArray(queriesResponse).length === 0
-  ) {
-    return (
-      <EmptyState
-        size="large"
-        title={t('Your chart is ready to go!')}
-        description={
-          <span>
-            {t(
-              'Click on "Create chart" button in the control panel on the left to preview a visualization or',
-            )}{' '}
-            <span role="button" tabIndex={0} onClick={onQuery}>
-              {t('click here')}
+    if (errorMessage && ensureIsArray(queriesResponse).length === 0) {
+      return (
+        <EmptyState
+          size="large"
+          title={t('Add required control values to preview chart')}
+          description={getChartRequiredFieldsMissingMessage(true)}
+          image="chart.svg"
+        />
+      );
+    }
+    if (
+      !isLoading &&
+      !chartAlert &&
+      !errorMessage &&
+      chartIsStale &&
+      ensureIsArray(queriesResponse).length === 0
+    ) {
+      return (
+        <EmptyState
+          size="large"
+          title={t('Your chart is ready to go!')}
+          description={
+            <span>
+              {t(
+                'Click on "Create chart" button in the control panel on the left to preview a visualization or',
+              )}{' '}
+              <span role="button" tabIndex={0} onClick={this.props.onQuery}>
+                {t('click here')}
+              </span>
+              .
             </span>
-            .
-          </span>
-        }
-        image="chart.svg"
-      />
+          }
+          image="chart.svg"
+        />
+      );
+    }
+
+    return (
+      <ErrorBoundary
+        onError={this.handleRenderContainerFailure}
+        showMessage={false}
+      >
+        <Styles
+          data-ui-anchor="chart"
+          className="chart-container"
+          data-test="chart-container"
+          height={height}
+          width={width}
+        >
+          {showSpinner
+            ? this.renderSpinner(databaseName)
+            : this.renderChartContainer()}
+        </Styles>
+      </ErrorBoundary>
     );
   }
-
-  return (
-    <ErrorBoundary onError={handleRenderContainerFailure} showMessage={false}>
-      <Styles
-        data-ui-anchor="chart"
-        className="chart-container"
-        data-test="chart-container"
-        height={height}
-        width={width}
-      >
-        {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
-      </Styles>
-    </ErrorBoundary>
-  );
 }
 export default Chart;

superset-frontend/src/components/Chart/ChartRenderer.test.tsx

@@ -394,9 +394,7 @@ test('renders chart during loading when suppressLoadingSpinner has valid data',
     queriesResponse: [{ data: [{ value: 1 }] }],
   };
 
-  const { getByTestId } = render(
-    <ChartRenderer {...(props as ChartRendererProps)} />,
-  );
+  const { getByTestId } = render(<ChartRenderer {...props} />);
   expect(getByTestId('mock-super-chart')).toBeInTheDocument();
   expect(getByTestId('mock-super-chart')).toHaveAttribute(
     'data-is-refreshing',
@@ -413,9 +411,7 @@ test('does not mark chart as refreshing when loading is not in progress', () =>
     queriesResponse: [{ data: [{ value: 1 }] }],
   };
 
-  const { getByTestId } = render(
-    <ChartRenderer {...(props as ChartRendererProps)} />,
-  );
+  const { getByTestId } = render(<ChartRenderer {...props} />);
   expect(getByTestId('mock-super-chart')).toHaveAttribute(
     'data-is-refreshing',
     'false',
@@ -431,9 +427,7 @@ test('does not mark chart as refreshing when spinner suppression is disabled', (
     queriesResponse: [{ data: [{ value: 1 }] }],
   };
 
-  const { getByTestId } = render(
-    <ChartRenderer {...(props as ChartRendererProps)} />,
-  );
+  const { getByTestId } = render(<ChartRenderer {...props} />);
   expect(getByTestId('mock-super-chart')).toHaveAttribute(
     'data-is-refreshing',
     'false',
@@ -449,8 +443,6 @@ test('does not render chart during loading when last data has errors', () => {
     queriesResponse: [{ error: 'bad' }],
   };
 
-  const { queryByTestId } = render(
-    <ChartRenderer {...(props as ChartRendererProps)} />,
-  );
+  const { queryByTestId } = render(<ChartRenderer {...props} />);
   expect(queryByTestId('mock-super-chart')).not.toBeInTheDocument();
 });

superset-frontend/src/components/Chart/ChartRenderer.tsx

@@ -16,17 +16,8 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { snakeCase, cloneDeep } from 'lodash';
-import {
-  useCallback,
-  useEffect,
-  useState,
-  useRef,
-  useMemo,
-  MouseEvent,
-  ReactNode,
-  memo,
-} from 'react';
+import { snakeCase, isEqual, cloneDeep } from 'lodash';
+import { createRef, Component, RefObject, MouseEvent, ReactNode } from 'react';
 import {
   SuperChart,
   Behavior,
@@ -46,7 +37,6 @@ import {
 } from '@superset-ui/core';
 import { logging } from '@apache-superset/core/utils';
 import { t } from '@apache-superset/core/translation';
-import { useTheme } from '@apache-superset/core/theme';
 import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils';
 import { EmptyState } from '@superset-ui/core/components';
 import { ChartSource } from 'src/types/ChartSource';
@@ -147,6 +137,14 @@ export interface ChartRendererProps {
   suppressLoadingSpinner?: boolean;
 }
 
+// State interface
+interface ChartRendererState {
+  showContextMenu: boolean;
+  inContextMenu: boolean;
+  legendState: LegendState | undefined;
+  legendIndex: number;
+}
+
 // Hooks interface
 interface ChartHooks {
   onAddFilter: (
@@ -177,370 +175,402 @@ const BIG_NO_RESULT_MIN_HEIGHT = 220;
 
 const behaviors = [Behavior.InteractiveChart];
 
-interface ChartRendererState {
-  showContextMenu: boolean;
-  inContextMenu: boolean;
-  legendState: LegendState | undefined;
-  legendIndex: number;
-}
+const defaultProps: Partial<ChartRendererProps> = {
+  addFilter: () => BLANK,
+  onFilterMenuOpen: () => BLANK,
+  onFilterMenuClose: () => BLANK,
+  initialValues: BLANK,
+  setControlValue: () => {},
+  triggerRender: false,
+};
 
-function ChartRendererComponent({
-  addFilter = () => BLANK,
-  onFilterMenuOpen = () => BLANK,
-  onFilterMenuClose = () => BLANK,
-  initialValues = BLANK,
-  setControlValue = () => {},
-  triggerRender = false,
-  ...restProps
-}: ChartRendererProps): JSX.Element | null {
-  const {
-    annotationData,
-    actions,
-    chartId,
-    datasource,
-    formData,
-    latestQueryFormData,
-    height,
-    width,
-    vizType: propVizType,
-    chartAlert,
-    chartStatus,
-    queriesResponse,
-    chartIsStale,
-    ownState,
-    filterState,
-    postTransformProps,
-    source,
-    emitCrossFilters,
-    onChartStateChange,
-  } = restProps;
+class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
+  static defaultProps = defaultProps;
 
-  const theme = useTheme();
+  private hasQueryResponseChange: boolean;
 
-  const suppressContextMenu = getChartMetadataRegistry().get(
-    formData.viz_type ?? propVizType,
-  )?.suppressContextMenu;
+  private contextMenuRef: RefObject<ChartContextMenuRef>;
 
-  const [state, setState] = useState<ChartRendererState>({
-    showContextMenu:
-      source === ChartSource.Dashboard &&
-      !suppressContextMenu &&
-      isFeatureEnabled(FeatureFlag.DrillToDetail),
-    inContextMenu: false,
-    legendState: undefined,
-    legendIndex: 0,
-  });
+  private hooks: ChartHooks;
 
-  const hasQueryResponseChangeRef = useRef(false);
-  const renderStartTimeRef = useRef(0);
-  const contextMenuRef = useRef<ChartContextMenuRef>(null);
+  private mutableQueriesResponse: QueryData[] | null | undefined;
 
-  // Results are "ready" when we have a non-error queriesResponse and the
-  // chartStatus reflects it. This mirrors the gating logic from the former
-  // shouldComponentUpdate implementation.
-  const resultsReady =
-    queriesResponse &&
-    ['success', 'rendered'].indexOf(chartStatus as string) > -1 &&
-    !queriesResponse?.[0]?.error;
+  private renderStartTime: number;
 
-  // Track whether queriesResponse changed since the previous render so that
-  // handleRenderSuccess / handleRenderFailure know whether to log render time.
-  // Updating a ref during render is safe when the value doesn't affect the
-  // render output (here it's read asynchronously from SuperChart callbacks).
-  const prevQueriesResponseRef = useRef<QueryData[] | null | undefined>(
-    queriesResponse,
-  );
-  if (resultsReady) {
-    hasQueryResponseChangeRef.current =
-      queriesResponse !== prevQueriesResponseRef.current;
+  constructor(props: ChartRendererProps) {
+    super(props);
+    const suppressContextMenu = getChartMetadataRegistry().get(
+      props.formData.viz_type ?? props.vizType,
+    )?.suppressContextMenu;
+    this.state = {
+      showContextMenu:
+        props.source === ChartSource.Dashboard &&
+        !suppressContextMenu &&
+        isFeatureEnabled(FeatureFlag.DrillToDetail),
+      inContextMenu: false,
+      legendState: undefined,
+      legendIndex: 0,
+    };
+    this.hasQueryResponseChange = false;
+    this.renderStartTime = 0;
+
+    this.contextMenuRef = createRef<ChartContextMenuRef>();
+
+    this.handleAddFilter = this.handleAddFilter.bind(this);
+    this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
+    this.handleRenderFailure = this.handleRenderFailure.bind(this);
+    this.handleSetControlValue = this.handleSetControlValue.bind(this);
+    this.handleOnContextMenu = this.handleOnContextMenu.bind(this);
+    this.handleContextMenuSelected = this.handleContextMenuSelected.bind(this);
+    this.handleContextMenuClosed = this.handleContextMenuClosed.bind(this);
+    this.handleLegendStateChanged = this.handleLegendStateChanged.bind(this);
+    this.onContextMenuFallback = this.onContextMenuFallback.bind(this);
+    this.handleLegendScroll = this.handleLegendScroll.bind(this);
+
+    this.hooks = {
+      onAddFilter: this.handleAddFilter,
+      onContextMenu: this.state.showContextMenu
+        ? this.handleOnContextMenu
+        : undefined,
+      onError: this.handleRenderFailure,
+      setControlValue: this.handleSetControlValue,
+      onFilterMenuOpen: this.props.onFilterMenuOpen,
+      onFilterMenuClose: this.props.onFilterMenuClose,
+      onLegendStateChanged: this.handleLegendStateChanged,
+      setDataMask: (dataMask: DataMask) => {
+        this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
+      },
+      onLegendScroll: this.handleLegendScroll,
+      onChartStateChange: this.props.onChartStateChange,
+    };
+
+    // TODO: queriesResponse comes from Redux store but it's being edited by
+    // the plugins, hence we need to clone it to avoid state mutation
+    // until we change the reducers to use Redux Toolkit with Immer
+    this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
   }
-  useEffect(() => {
-    prevQueriesResponseRef.current = queriesResponse;
-  }, [queriesResponse]);
 
-  // Clone queriesResponse to protect against plugin mutation of Redux state.
-  // TODO: remove once reducers use Redux Toolkit with Immer.
-  const mutableQueriesResponse = useMemo(
-    () => cloneDeep(queriesResponse),
-    [queriesResponse],
-  );
+  shouldComponentUpdate(
+    nextProps: ChartRendererProps,
+    nextState: ChartRendererState,
+  ): boolean {
+    const resultsReady =
+      nextProps.queriesResponse &&
+      ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
+      !nextProps.queriesResponse?.[0]?.error;
 
-  // Handler functions
-  const handleAddFilter = useCallback(
-    (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
-      addFilter?.(col, vals, merge, refresh);
-    },
-    [addFilter],
-  );
+    if (resultsReady) {
+      if (!isEqual(this.state, nextState)) {
+        return true;
+      }
+      this.hasQueryResponseChange =
+        nextProps.queriesResponse !== this.props.queriesResponse;
 
-  const handleRenderSuccess = useCallback((): void => {
+      if (this.hasQueryResponseChange) {
+        this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
+      }
+
+      // Check if any matrixify-related properties have changed
+      const hasMatrixifyChanges = (): boolean => {
+        const nextFormData = nextProps.formData as JsonObject;
+        const currentFormData = this.props.formData as JsonObject;
+        const isMatrixifyEnabled =
+          nextFormData.matrixify_enable === true &&
+          ((nextFormData.matrixify_mode_rows !== undefined &&
+            nextFormData.matrixify_mode_rows !== 'disabled') ||
+            (nextFormData.matrixify_mode_columns !== undefined &&
+              nextFormData.matrixify_mode_columns !== 'disabled'));
+        if (!isMatrixifyEnabled) return false;
+
+        // Check all matrixify-related properties
+        const matrixifyKeys = Object.keys(nextFormData).filter(key =>
+          key.startsWith('matrixify_'),
+        );
+
+        return matrixifyKeys.some(
+          key => !isEqual(nextFormData[key], currentFormData[key]),
+        );
+      };
+
+      const nextFormData = nextProps.formData as JsonObject;
+      const currentFormData = this.props.formData as JsonObject;
+
+      return (
+        this.hasQueryResponseChange ||
+        !isEqual(nextProps.datasource, this.props.datasource) ||
+        nextProps.annotationData !== this.props.annotationData ||
+        nextProps.ownState !== this.props.ownState ||
+        nextProps.filterState !== this.props.filterState ||
+        nextProps.height !== this.props.height ||
+        nextProps.width !== this.props.width ||
+        nextProps.triggerRender === true ||
+        nextProps.labelsColor !== this.props.labelsColor ||
+        nextProps.labelsColorMap !== this.props.labelsColorMap ||
+        nextFormData.color_scheme !== currentFormData.color_scheme ||
+        nextFormData.stack !== currentFormData.stack ||
+        nextFormData.subcategories !== currentFormData.subcategories ||
+        nextProps.cacheBusterProp !== this.props.cacheBusterProp ||
+        nextProps.emitCrossFilters !== this.props.emitCrossFilters ||
+        nextProps.postTransformProps !== this.props.postTransformProps ||
+        hasMatrixifyChanges()
+      );
+    }
+    return false;
+  }
+
+  handleAddFilter(
+    col: string,
+    vals: FilterValue[],
+    merge = true,
+    refresh = true,
+  ): void {
+    this.props.addFilter?.(col, vals, merge, refresh);
+  }
+
+  handleRenderSuccess(): void {
+    const { actions, chartStatus, chartId, vizType } = this.props;
     if (['loading', 'rendered'].indexOf(chartStatus as string) < 0) {
       actions.chartRenderingSucceeded(chartId);
     }
 
     // only log chart render time which is triggered by query results change
-    if (hasQueryResponseChangeRef.current) {
+    // currently we don't log chart re-render time, like window resize etc
+    if (this.hasQueryResponseChange) {
       actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
         slice_id: chartId,
-        viz_type: propVizType,
-        start_offset: renderStartTimeRef.current,
+        viz_type: vizType,
+        start_offset: this.renderStartTime,
         ts: new Date().getTime(),
-        duration: Logger.getTimestamp() - renderStartTimeRef.current,
+        duration: Logger.getTimestamp() - this.renderStartTime,
       });
     }
-  }, [actions, chartId, chartStatus, propVizType]);
+  }
 
-  const handleRenderFailure = useCallback(
-    (error: Error, info: { componentStack: string } | null): void => {
-      logging.warn(error);
-      actions.chartRenderingFailed(
-        error.toString(),
-        chartId,
-        info ? info.componentStack : null,
-      );
+  handleRenderFailure(
+    error: Error,
+    info: { componentStack: string } | null,
+  ): void {
+    const { actions, chartId } = this.props;
+    logging.warn(error);
+    actions.chartRenderingFailed(
+      error.toString(),
+      chartId,
+      info ? info.componentStack : null,
+    );
 
-      // only trigger render log when query is changed
-      if (hasQueryResponseChangeRef.current) {
-        actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
-          slice_id: chartId,
-          has_err: true,
-          error_details: error.toString(),
-          start_offset: renderStartTimeRef.current,
-          ts: new Date().getTime(),
-          duration: Logger.getTimestamp() - renderStartTimeRef.current,
-        });
-      }
-    },
-    [actions, chartId],
-  );
+    // only trigger render log when query is changed
+    if (this.hasQueryResponseChange) {
+      actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+        slice_id: chartId,
+        has_err: true,
+        error_details: error.toString(),
+        start_offset: this.renderStartTime,
+        ts: new Date().getTime(),
+        duration: Logger.getTimestamp() - this.renderStartTime,
+      });
+    }
+  }
 
-  const handleSetControlValue = useCallback(
-    (name: string, value: unknown): void => {
-      if (setControlValue) {
-        setControlValue(name, value);
-      }
-    },
-    [setControlValue],
-  );
+  handleSetControlValue(name: string, value: unknown): void {
+    const { setControlValue } = this.props;
+    if (setControlValue) {
+      setControlValue(name, value);
+    }
+  }
 
-  const handleOnContextMenu = useCallback(
-    (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
-      contextMenuRef.current?.open(offsetX, offsetY, filters);
-      setState(prev => ({ ...prev, inContextMenu: true }));
-    },
-    [contextMenuRef],
-  );
+  handleOnContextMenu(
+    offsetX: number,
+    offsetY: number,
+    filters?: ContextMenuFilters,
+  ): void {
+    this.contextMenuRef.current?.open(offsetX, offsetY, filters);
+    this.setState({ inContextMenu: true });
+  }
 
-  const handleContextMenuSelected = useCallback((): void => {
-    setState(prev => ({ ...prev, inContextMenu: false }));
-  }, []);
+  handleContextMenuSelected(): void {
+    this.setState({ inContextMenu: false });
+  }
 
-  const handleContextMenuClosed = useCallback((): void => {
-    setState(prev => ({ ...prev, inContextMenu: false }));
-  }, []);
+  handleContextMenuClosed(): void {
+    this.setState({ inContextMenu: false });
+  }
 
-  const handleLegendStateChanged = useCallback(
-    (legendState: LegendState): void => {
-      setState(prev => ({ ...prev, legendState }));
-    },
-    [],
-  );
-
-  const handleLegendScroll = useCallback((legendIndex: number): void => {
-    setState(prev => ({ ...prev, legendIndex }));
-  }, []);
+  handleLegendStateChanged(legendState: LegendState): void {
+    this.setState({ legendState });
+  }
 
   // When viz plugins don't handle `contextmenu` event, fallback handler
   // calls `handleOnContextMenu` with no `filters` param.
-  const onContextMenuFallback = useCallback(
-    (event: MouseEvent<HTMLDivElement>): void => {
-      if (!state.inContextMenu) {
-        event.preventDefault();
-        handleOnContextMenu(event.clientX, event.clientY);
-      }
-    },
-    [handleOnContextMenu, state.inContextMenu],
-  );
-
-  const setDataMaskCallback = useCallback(
-    (dataMask: DataMask) => {
-      actions?.updateDataMask?.(chartId, dataMask);
-    },
-    [actions, chartId],
-  );
-
-  // Hooks object - memoized
-  const hooks = useMemo<ChartHooks>(
-    () => ({
-      onAddFilter: handleAddFilter,
-      onContextMenu: state.showContextMenu ? handleOnContextMenu : undefined,
-      onError: handleRenderFailure,
-      setControlValue: handleSetControlValue,
-      onFilterMenuOpen,
-      onFilterMenuClose,
-      onLegendStateChanged: handleLegendStateChanged,
-      setDataMask: setDataMaskCallback,
-      onLegendScroll: handleLegendScroll,
-      onChartStateChange,
-    }),
-    [
-      handleAddFilter,
-      handleLegendScroll,
-      handleLegendStateChanged,
-      handleOnContextMenu,
-      handleRenderFailure,
-      handleSetControlValue,
-      onChartStateChange,
-      onFilterMenuClose,
-      onFilterMenuOpen,
-      setDataMaskCallback,
-      state.showContextMenu,
-    ],
-  );
-
-  const hasAnyErrors = queriesResponse?.some(item => item?.error);
-  const hasValidPreviousData =
-    (queriesResponse?.length ?? 0) > 0 && !hasAnyErrors;
-
-  if (!!chartAlert || chartStatus === null) {
-    return null;
-  }
-
-  if (chartStatus === 'loading') {
-    if (!restProps.suppressLoadingSpinner || !hasValidPreviousData) {
-      return null;
+  onContextMenuFallback(event: MouseEvent<HTMLDivElement>): void {
+    if (!this.state.inContextMenu) {
+      event.preventDefault();
+      this.handleOnContextMenu(event.clientX, event.clientY);
     }
   }
 
-  renderStartTimeRef.current = Logger.getTimestamp();
-
-  const currentFormData =
-    chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
-  const vizType = currentFormData.viz_type || propVizType;
-
-  // It's bad practice to use unprefixed `vizType` as classnames for chart
-  // container. It may cause css conflicts as in the case of legacy table chart.
-  // When migrating charts, we should gradually add a `superset-chart-` prefix
-  // to each one of them.
-  const snakeCaseVizType = snakeCase(vizType);
-  const chartClassName =
-    vizType === VizType.Table
-      ? `superset-chart-${snakeCaseVizType}`
-      : snakeCaseVizType;
-
-  const webpackHash =
-    process.env.WEBPACK_MODE === 'development'
-      ? `-${
-          // eslint-disable-next-line camelcase
-          typeof __webpack_require__ !== 'undefined' &&
-          // eslint-disable-next-line camelcase, no-undef
-          typeof __webpack_require__.h === 'function' &&
-          // eslint-disable-next-line no-undef, camelcase
-          __webpack_require__.h()
-        }`
-      : '';
-
-  let noResultsComponent: ReactNode;
-  const noResultTitle = t('No results were returned for this query');
-  const noResultDescription =
-    source === ChartSource.Explore
-      ? t(
-          'Make sure that the controls are configured properly and the datasource contains data for the selected time range',
-        )
-      : undefined;
-  const noResultImage = 'chart.svg';
-  if (
-    (width ?? 0) > BIG_NO_RESULT_MIN_WIDTH &&
-    (height ?? 0) > BIG_NO_RESULT_MIN_HEIGHT
-  ) {
-    noResultsComponent = (
-      <EmptyState
-        size="large"
-        title={noResultTitle}
-        description={noResultDescription}
-        image={noResultImage}
-      />
-    );
-  } else {
-    noResultsComponent = (
-      <EmptyState title={noResultTitle} image={noResultImage} size="small" />
-    );
+  handleLegendScroll(legendIndex: number): void {
+    this.setState({ legendIndex });
   }
 
-  // Check for Behavior.DRILL_TO_DETAIL to tell if chart can receive Drill to
-  // Detail props or if it'll cause side-effects (e.g. excessive re-renders).
-  const drillToDetailProps = getChartMetadataRegistry()
-    .get(vizType)
-    ?.behaviors.find(behavior => behavior === Behavior.DrillToDetail)
-    ? { inContextMenu: state.inContextMenu }
-    : {};
-  // By pass no result component when server pagination is enabled & the table has:
-  // - a backend search query, OR
-  // - non-empty AG Grid filter model
-  const hasSearchText = (ownState?.searchText?.length || 0) > 0;
-  const hasAgGridFilters =
-    ownState?.agGridFilterModel &&
-    Object.keys(ownState.agGridFilterModel).length > 0;
+  render(): ReactNode {
+    const { chartAlert, chartStatus, chartId, emitCrossFilters } = this.props;
 
-  const currentFormDataExtended = currentFormData as JsonObject;
-  const bypassNoResult = !(
-    currentFormDataExtended?.server_pagination &&
-    (hasSearchText || hasAgGridFilters)
-  );
+    const hasAnyErrors = this.props.queriesResponse?.some(item => item?.error);
+    const hasValidPreviousData =
+      (this.props.queriesResponse?.length ?? 0) > 0 && !hasAnyErrors;
 
-  return (
-    <>
-      {state.showContextMenu && (
-        <ChartContextMenu
-          ref={contextMenuRef}
-          id={chartId}
-          formData={currentFormData as QueryFormData}
-          onSelection={handleContextMenuSelected}
-          onClose={handleContextMenuClosed}
+    if (!!chartAlert || chartStatus === null) {
+      return null;
+    }
+
+    if (chartStatus === 'loading') {
+      if (!this.props.suppressLoadingSpinner || !hasValidPreviousData) {
+        return null;
+      }
+    }
+
+    this.renderStartTime = Logger.getTimestamp();
+
+    const {
+      width,
+      height,
+      datasource,
+      annotationData,
+      initialValues,
+      ownState,
+      filterState,
+      chartIsStale,
+      formData,
+      latestQueryFormData,
+      postTransformProps,
+    } = this.props;
+
+    const currentFormData =
+      chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
+    const vizType = currentFormData.viz_type || this.props.vizType;
+
+    // It's bad practice to use unprefixed `vizType` as classnames for chart
+    // container. It may cause css conflicts as in the case of legacy table chart.
+    // When migrating charts, we should gradually add a `superset-chart-` prefix
+    // to each one of them.
+    const snakeCaseVizType = snakeCase(vizType);
+    const chartClassName =
+      vizType === VizType.Table
+        ? `superset-chart-${snakeCaseVizType}`
+        : snakeCaseVizType;
+
+    const webpackHash =
+      process.env.WEBPACK_MODE === 'development'
+        ? `-${
+            // eslint-disable-next-line camelcase
+            typeof __webpack_require__ !== 'undefined' &&
+            // eslint-disable-next-line camelcase, no-undef
+            typeof __webpack_require__.h === 'function' &&
+            // eslint-disable-next-line no-undef, camelcase
+            __webpack_require__.h()
+          }`
+        : '';
+
+    let noResultsComponent: ReactNode;
+    const noResultTitle = t('No results were returned for this query');
+    const noResultDescription =
+      this.props.source === ChartSource.Explore
+        ? t(
+            'Make sure that the controls are configured properly and the datasource contains data for the selected time range',
+          )
+        : undefined;
+    const noResultImage = 'chart.svg';
+    if (
+      (width ?? 0) > BIG_NO_RESULT_MIN_WIDTH &&
+      (height ?? 0) > BIG_NO_RESULT_MIN_HEIGHT
+    ) {
+      noResultsComponent = (
+        <EmptyState
+          size="large"
+          title={noResultTitle}
+          description={noResultDescription}
+          image={noResultImage}
         />
-      )}
-      <div
-        onContextMenu={
-          state.showContextMenu ? onContextMenuFallback : undefined
-        }
-      >
-        <SuperChart
-          disableErrorBoundary
-          key={`${chartId}${webpackHash}`}
-          id={`chart-id-${chartId}`}
-          className={chartClassName}
-          chartType={vizType}
-          width={width}
-          height={height}
-          theme={theme}
-          annotationData={annotationData}
-          datasource={datasource}
-          initialValues={initialValues}
-          formData={currentFormData}
-          ownState={ownState}
-          filterState={filterState}
-          hooks={hooks as unknown as Parameters<typeof SuperChart>[0]['hooks']}
-          behaviors={behaviors}
-          queriesData={mutableQueriesResponse ?? undefined}
-          onRenderSuccess={handleRenderSuccess}
-          onRenderFailure={handleRenderFailure}
-          noResults={noResultsComponent}
-          postTransformProps={postTransformProps}
-          emitCrossFilters={emitCrossFilters}
-          legendState={state.legendState}
-          enableNoResults={bypassNoResult}
-          legendIndex={state.legendIndex}
-          isRefreshing={
-            Boolean(restProps.suppressLoadingSpinner) &&
-            chartStatus === 'loading'
+      );
+    } else {
+      noResultsComponent = (
+        <EmptyState title={noResultTitle} image={noResultImage} size="small" />
+      );
+    }
+
+    // Check for Behavior.DRILL_TO_DETAIL to tell if chart can receive Drill to
+    // Detail props or if it'll cause side-effects (e.g. excessive re-renders).
+    const drillToDetailProps = getChartMetadataRegistry()
+      .get(vizType)
+      ?.behaviors.find(behavior => behavior === Behavior.DrillToDetail)
+      ? { inContextMenu: this.state.inContextMenu }
+      : {};
+    // By pass no result component when server pagination is enabled & the table has:
+    // - a backend search query, OR
+    // - non-empty AG Grid filter model
+    const hasSearchText = (ownState?.searchText?.length || 0) > 0;
+    const hasAgGridFilters =
+      ownState?.agGridFilterModel &&
+      Object.keys(ownState.agGridFilterModel).length > 0;
+
+    const currentFormDataExtended = currentFormData as JsonObject;
+    const bypassNoResult = !(
+      currentFormDataExtended?.server_pagination &&
+      (hasSearchText || hasAgGridFilters)
+    );
+
+    return (
+      <>
+        {this.state.showContextMenu && (
+          <ChartContextMenu
+            ref={this.contextMenuRef}
+            id={chartId}
+            formData={currentFormData as QueryFormData}
+            onSelection={this.handleContextMenuSelected}
+            onClose={this.handleContextMenuClosed}
+          />
+        )}
+        <div
+          onContextMenu={
+            this.state.showContextMenu ? this.onContextMenuFallback : undefined
           }
-          {...drillToDetailProps}
-        />
-      </div>
-    </>
-  );
+        >
+          <SuperChart
+            disableErrorBoundary
+            key={`${chartId}${webpackHash}`}
+            id={`chart-id-${chartId}`}
+            className={chartClassName}
+            chartType={vizType}
+            width={width}
+            height={height}
+            annotationData={annotationData}
+            datasource={datasource}
+            initialValues={initialValues}
+            formData={currentFormData}
+            ownState={ownState}
+            filterState={filterState}
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            hooks={this.hooks as any}
+            behaviors={behaviors}
+            queriesData={this.mutableQueriesResponse ?? undefined}
+            onRenderSuccess={this.handleRenderSuccess}
+            onRenderFailure={this.handleRenderFailure}
+            noResults={noResultsComponent}
+            postTransformProps={postTransformProps}
+            emitCrossFilters={emitCrossFilters}
+            legendState={this.state.legendState}
+            enableNoResults={bypassNoResult}
+            legendIndex={this.state.legendIndex}
+            isRefreshing={
+              Boolean(this.props.suppressLoadingSpinner) &&
+              chartStatus === 'loading'
+            }
+            {...drillToDetailProps}
+          />
+        </div>
+      </>
+    );
+  }
 }
 
-const ChartRenderer = memo(ChartRendererComponent);
-
 export default ChartRenderer;

superset-frontend/src/components/Chart/DrillBy/DrillByChart.tsx

@@ -23,7 +23,7 @@ import {
   SuperChart,
   ContextMenuFilters,
 } from '@superset-ui/core';
-import { css, useTheme } from '@apache-superset/core/theme';
+import { css } from '@apache-superset/core/theme';
 import { Dataset } from '../types';
 
 interface DrillByChartProps {
@@ -45,7 +45,6 @@ export default function DrillByChart({
   onContextMenu,
   inContextMenu,
 }: DrillByChartProps) {
-  const theme = useTheme();
   const hooks = useMemo(() => ({ onContextMenu }), [onContextMenu]);
 
   return (
@@ -68,7 +67,6 @@ export default function DrillByChart({
         inContextMenu={inContextMenu}
         height="100%"
         width="100%"
-        theme={theme}
       />
     </div>
   );