fix(tests): update animation-wait tests to patch _browser_manager and fix E501

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

pyproject.toml

@@ -375,6 +375,7 @@ select = [
 
 ignore = [
     "S101",
+    "PT001",  # pytest-fixture-incorrect-parentheses-style: different ruff versions disagree
     "PT006",
     "T201",
     "N999",

superset-frontend/cypress-base/cypress/e2e/database/modal.test.ts

@@ -1,118 +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 { DATABASE_LIST } from 'cypress/utils/urls';
-
-function closeModal() {
-  cy.get('body').then($body => {
-    if ($body.find('[data-test="database-modal"]').length) {
-      cy.get('[aria-label="Close"]').eq(1).click();
-    }
-  });
-}
-
-describe('Add database', () => {
-  before(() => {
-    cy.visit(DATABASE_LIST);
-  });
-
-  beforeEach(() => {
-    cy.intercept('POST', '**/api/v1/database/validate_parameters/**').as(
-      'validateParams',
-    );
-    cy.intercept('POST', '**/api/v1/database/').as('createDb');
-
-    closeModal();
-    cy.getBySel('btn-create-database').click();
-  });
-
-  it('should open dynamic form', () => {
-    cy.get('.preferred > :nth-child(1)').click();
-
-    cy.get('input[name="host"]').should('have.value', '');
-    cy.get('input[name="port"]').should('have.value', '');
-    cy.get('input[name="database"]').should('have.value', '');
-    cy.get('input[name="username"]').should('have.value', '');
-    cy.get('input[name="password"]').should('have.value', '');
-    cy.get('input[name="database_name"]').should('have.value', '');
-  });
-
-  it('should open sqlalchemy form', () => {
-    cy.get('.preferred > :nth-child(1)').click();
-    cy.getBySel('sqla-connect-btn').click();
-
-    cy.getBySel('database-name-input').should('be.visible');
-    cy.getBySel('sqlalchemy-uri-input').should('be.visible');
-  });
-
-  it('show error alerts on dynamic form for bad host', () => {
-    cy.get('.preferred > :nth-child(1)').click();
-
-    cy.get('input[name="host"]').type('badhost', { force: true });
-    cy.get('input[name="port"]').type('5432', { force: true });
-    cy.get('input[name="username"]').type('testusername', { force: true });
-    cy.get('input[name="database"]').type('testdb', { force: true });
-    cy.get('input[name="password"]').type('testpass', { force: true });
-
-    cy.get('body').click(0, 0);
-
-    cy.wait('@validateParams', { timeout: 30000 });
-
-    cy.getBySel('btn-submit-connection').should('not.be.disabled');
-    cy.getBySel('btn-submit-connection').click({ force: true });
-
-    cy.wait('@validateParams', { timeout: 30000 }).then(() => {
-      cy.wait('@createDb', { timeout: 60000 }).then(() => {
-        cy.contains(
-          '.ant-form-item-explain-error',
-          "The hostname provided can't be resolved",
-        ).should('exist');
-      });
-    });
-  });
-
-  it('show error alerts on dynamic form for bad port', () => {
-    cy.get('.preferred > :nth-child(1)').click();
-
-    cy.get('input[name="host"]').type('localhost', { force: true });
-    cy.get('body').click(0, 0);
-    cy.wait('@validateParams', { timeout: 30000 });
-
-    cy.get('input[name="port"]').type('5430', { force: true });
-    cy.get('input[name="database"]').type('testdb', { force: true });
-    cy.get('input[name="username"]').type('testusername', { force: true });
-
-    cy.wait('@validateParams', { timeout: 30000 });
-
-    cy.get('input[name="password"]').type('testpass', { force: true });
-    cy.wait('@validateParams');
-
-    cy.getBySel('btn-submit-connection').should('not.be.disabled');
-    cy.getBySel('btn-submit-connection').click({ force: true });
-    cy.wait('@validateParams', { timeout: 30000 }).then(() => {
-      cy.get('body').click(0, 0);
-      cy.getBySel('btn-submit-connection').click({ force: true });
-      cy.wait('@createDb', { timeout: 60000 }).then(() => {
-        cy.contains(
-          '.ant-form-item-explain-error',
-          'The port is closed',
-        ).should('exist');
-      });
-    });
-  });
-});

superset-frontend/package-lock.json

@@ -115,7 +115,7 @@
         "memoize-one": "^6.0.0",
         "mousetrap": "^1.6.5",
         "mustache": "^4.2.0",
-        "nanoid": "^5.1.11",
+        "nanoid": "^5.1.14",
         "ol": "^10.9.0",
         "query-string": "9.4.0",
         "re-resizable": "^6.11.2",
@@ -267,7 +267,7 @@
         "process": "^0.11.10",
         "react-dnd-test-backend": "^16.0.1",
         "react-refresh": "^0.18.0",
-        "react-resizable": "^4.0.1",
+        "react-resizable": "^4.0.2",
         "redux-mock-store": "^1.5.4",
         "source-map": "^0.7.6",
         "source-map-support": "^0.5.21",
@@ -26533,21 +26533,6 @@
         }
       }
     },
-    "node_modules/jsdom/node_modules/@noble/hashes": {
-      "version": "2.2.0",
-      "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz",
-      "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true,
-      "engines": {
-        "node": ">= 20.19.0"
-      },
-      "funding": {
-        "url": "https://paulmillr.com/funding/"
-      }
-    },
     "node_modules/jsdom/node_modules/css-tree": {
       "version": "3.2.1",
       "resolved": "https://registry.npmjs.org/css-tree/-/css-tree-3.2.1.tgz",
@@ -31452,9 +31437,9 @@
       }
     },
     "node_modules/nanoid": {
-      "version": "5.1.11",
-      "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-5.1.11.tgz",
-      "integrity": "sha512-v+KEsUv2ps74PaSKv0gHTxTCgMXOIfBEbaqa6w6ISIGC7ZsvHN4N9oJ8d4cmf0n5oTzQz2SLmThbQWhjd/8eKg==",
+      "version": "5.1.14",
+      "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-5.1.14.tgz",
+      "integrity": "sha512-5c8l8kVzqpnDPaicbEop/fV0Q1w16FmbWtVhMqugTozAwYdlIQojWH5a/M7UfziFmGdQRrUdV+EPzc9Xng3VAQ==",
       "funding": [
         {
           "type": "github",
@@ -36305,9 +36290,9 @@
       }
     },
     "node_modules/react-resizable": {
-      "version": "4.0.1",
-      "resolved": "https://registry.npmjs.org/react-resizable/-/react-resizable-4.0.1.tgz",
-      "integrity": "sha512-FR25Rcfxpi1iKiC7taIrqc1Tz6VnslqM94/IrA1LxoM5C3ap2EqaKLnCit/aKrcn3H4wfzO0nFBadFBc+SzEWA==",
+      "version": "4.0.2",
+      "resolved": "https://registry.npmjs.org/react-resizable/-/react-resizable-4.0.2.tgz",
+      "integrity": "sha512-jZD9ghYRmyJCw0+awYctSZ+9pmX1WXQvzDrTovELYc8obC/BShTI2r4c14LIVzeQ+vJZNb0yKM7bG2eqv7Vfyg==",
       "dev": true,
       "license": "MIT",
       "dependencies": {
@@ -43585,21 +43570,6 @@
         }
       }
     },
-    "node_modules/whatwg-url/node_modules/@noble/hashes": {
-      "version": "2.2.0",
-      "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz",
-      "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true,
-      "engines": {
-        "node": ">= 20.19.0"
-      },
-      "funding": {
-        "url": "https://paulmillr.com/funding/"
-      }
-    },
     "node_modules/whatwg-url/node_modules/webidl-conversions": {
       "version": "8.0.1",
       "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-8.0.1.tgz",
@@ -44851,7 +44821,7 @@
         "parse-ms": "^4.0.0",
         "re-resizable": "^6.11.2",
         "react-ace": "^14.0.1",
-        "react-draggable": "^4.6.0",
+        "react-draggable": "^4.7.0",
         "react-error-boundary": "6.0.0",
         "react-js-cron": "^5.2.0",
         "react-markdown": "^8.0.7",
@@ -44970,9 +44940,9 @@
       }
     },
     "packages/superset-ui-core/node_modules/react-draggable": {
-      "version": "4.6.0",
-      "resolved": "https://registry.npmjs.org/react-draggable/-/react-draggable-4.6.0.tgz",
-      "integrity": "sha512-g4vqY53xhmPrBnZvGP+1YQV0eYnB3o0VLzoi6q2IpwnQrxIZ34tYRKpVtsWIXPg4D/pvLn+oYCW5gOK2cWIrgA==",
+      "version": "4.7.0",
+      "resolved": "https://registry.npmjs.org/react-draggable/-/react-draggable-4.7.0.tgz",
+      "integrity": "sha512-kTpANmKWVnFXiZ76Ag2ZowiFStuBYnJ606PI1TbUsOg29/400/JNIxI9+CuenhiAqFuXWJffz6F4UI3R51kUug==",
       "license": "MIT",
       "dependencies": {
         "clsx": "^2.1.1",
@@ -45681,6 +45651,36 @@
       "version": "1.0.0",
       "extraneous": true,
       "license": "Apache-2.0"
+    },
+    "node_modules/jsdom/node_modules/@noble/hashes": {
+      "version": "2.2.0",
+      "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz",
+      "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==",
+      "dev": true,
+      "license": "MIT",
+      "optional": true,
+      "peer": true,
+      "engines": {
+        "node": ">= 20.19.0"
+      },
+      "funding": {
+        "url": "https://paulmillr.com/funding/"
+      }
+    },
+    "node_modules/whatwg-url/node_modules/@noble/hashes": {
+      "version": "2.2.0",
+      "resolved": "https://registry.npmjs.org/@noble/hashes/-/hashes-2.2.0.tgz",
+      "integrity": "sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==",
+      "dev": true,
+      "license": "MIT",
+      "optional": true,
+      "peer": true,
+      "engines": {
+        "node": ">= 20.19.0"
+      },
+      "funding": {
+        "url": "https://paulmillr.com/funding/"
+      }
     }
   }
 }

superset-frontend/package.json

@@ -198,7 +198,7 @@
     "memoize-one": "^6.0.0",
     "mousetrap": "^1.6.5",
     "mustache": "^4.2.0",
-    "nanoid": "^5.1.11",
+    "nanoid": "^5.1.14",
     "ol": "^10.9.0",
     "query-string": "9.4.0",
     "re-resizable": "^6.11.2",
@@ -350,7 +350,7 @@
     "process": "^0.11.10",
     "react-dnd-test-backend": "^16.0.1",
     "react-refresh": "^0.18.0",
-    "react-resizable": "^4.0.1",
+    "react-resizable": "^4.0.2",
     "redux-mock-store": "^1.5.4",
     "source-map": "^0.7.6",
     "source-map-support": "^0.5.21",

superset-frontend/packages/superset-ui-core/package.json

@@ -52,7 +52,7 @@
     "parse-ms": "^4.0.0",
     "re-resizable": "^6.11.2",
     "react-ace": "^14.0.1",
-    "react-draggable": "^4.6.0",
+    "react-draggable": "^4.7.0",
     "react-error-boundary": "6.0.0",
     "react-js-cron": "^5.2.0",
     "react-markdown": "^8.0.7",

superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx

@@ -113,7 +113,7 @@ const AsyncSelect = forwardRef(
       allowClear,
       allowNewOptions = false,
       ariaLabel,
-      autoClearSearchValue = false,
+      autoClearSearchValue = true,
       fetchOnlyOnSearch,
       filterOption = true,
       header = null,
@@ -267,6 +267,12 @@ const AsyncSelect = forwardRef(
         });
         fireOnChange();
       }
+      if (autoClearSearchValue) {
+        setInputValue('');
+        if (fetchOnlyOnSearch) {
+          setSelectOptions([]);
+        }
+      }
       onSelect?.(selectedItem, option);
     };
 

superset-frontend/packages/superset-ui-core/src/components/Select/Select.stories.tsx

@@ -404,7 +404,7 @@ AdvancedPlayground.args = {
   autoFocus: true,
   allowNewOptions: false,
   allowClear: false,
-  autoClearSearchValue: false,
+  autoClearSearchValue: true,
   allowSelectAll: true,
   disabled: false,
   invertSelection: false,

superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx

@@ -94,7 +94,7 @@ const Select = forwardRef(
       allowNewOptionsOnPaste = false,
       allowSelectAll = true,
       ariaLabel,
-      autoClearSearchValue = false,
+      autoClearSearchValue = true,
       filterOption = true,
       header = null,
       headerPosition = 'top',
@@ -334,6 +334,11 @@ const Select = forwardRef(
         });
         fireOnChange();
       }
+      if (autoClearSearchValue) {
+        setInputValue('');
+        setIsSearching(false);
+        setVisibleOptions(fullSelectOptions);
+      }
       onSelect?.(selectedItem, option);
     };
 

superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/transformProps.ts

@@ -186,7 +186,9 @@ export default function transformProps(
     showLabels,
     showLabelsThreshold,
     showTotal,
-    showNullValues,
+    // Default to true so charts saved before this control existed keep
+    // showing null values instead of silently hiding them on upgrade.
+    showNullValues = true,
     sliceId,
   } = formData;
   const {

superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts

@@ -40,7 +40,7 @@ export function treeBuilder(
 ): TreeNode[] {
   const [curGroupBy, ...restGroupby] = groupBy;
   const curData = _groupBy(data, curGroupBy);
-  return transform(
+  const nodes = transform(
     curData,
     (result, value, key) => {
       const name = curData[key][0][curGroupBy]!;
@@ -59,6 +59,9 @@ export function treeBuilder(
           result.push(item);
         });
       } else {
+        // Children are already null-filtered by the recursive call, so the
+        // parent's value/secondaryValue exclude hidden nulls. This keeps the
+        // parent arc sized to its visible children (no empty gap).
         const children = treeBuilder(
           value,
           restGroupby,
@@ -76,12 +79,9 @@ export function treeBuilder(
               0,
             )
           : metricValue;
-        const validChildren = filterNullNames
-          ? children.filter(child => child.name !== null)
-          : children;
         result.push({
           name,
-          children: validChildren,
+          children,
           value: metricValue,
           secondaryValue,
           groupBy: curGroupBy,
@@ -90,4 +90,13 @@ export function treeBuilder(
     },
     [] as TreeNode[],
   );
+  // Filter at every level so single-level charts and root nodes are covered,
+  // not just nested children. A parent whose children were all null-filtered
+  // is dropped too: keeping it would leave a zero-value arc that yields a NaN
+  // secondaryValue/value ratio for coloring and tooltips.
+  return filterNullNames
+    ? nodes.filter(
+        node => node.name !== null && node.children?.length !== 0,
+      )
+    : nodes;
 }

superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts

@@ -21,6 +21,13 @@ import { supersetTheme } from '@apache-superset/core/theme';
 import { EchartsSunburstChartProps } from '../../src/Sunburst/types';
 import transformProps from '../../src/Sunburst/transformProps';
 
+type SunburstSeries = {
+  label?: Record<string, unknown>;
+  data: { value: number }[];
+};
+const firstSeries = (echartOptions: unknown) =>
+  (echartOptions as { series: SunburstSeries[] }).series[0];
+
 const formData = {
   colorScheme: 'bnbColors',
   datasource: '3__table',
@@ -47,7 +54,52 @@ test('series label has no textBorderColor or textBorderWidth', () => {
   const { echartOptions } = transformProps(
     chartProps as EchartsSunburstChartProps,
   );
-  const series = (echartOptions as any).series[0];
+  const series = firstSeries(echartOptions);
   expect(series.label).not.toHaveProperty('textBorderColor');
   expect(series.label).not.toHaveProperty('textBorderWidth');
 });
+
+const nullValueProps = (showNullValues?: boolean) =>
+  new ChartProps({
+    formData: {
+      colorScheme: 'bnbColors',
+      datasource: '3__table',
+      columns: ['category'],
+      metric: 'sum__value',
+      ...(showNullValues === undefined ? {} : { showNullValues }),
+    },
+    width: 800,
+    height: 600,
+    queriesData: [
+      {
+        data: [
+          { category: 'A', sum__value: 10 },
+          { category: 'B', sum__value: 20 },
+          { category: null, sum__value: 5 },
+        ],
+      },
+    ],
+    theme: supersetTheme,
+  });
+
+const seriesValues = (props: ChartProps) => {
+  const { echartOptions } = transformProps(props as EchartsSunburstChartProps);
+  return firstSeries(echartOptions)
+    .data.map(node => node.value)
+    .sort((a, b) => a - b);
+};
+
+// Charts saved before the "Show Null Values" control existed have no
+// `showNullValues` in form data; they must keep showing nulls (non-breaking).
+test('keeps null values when showNullValues is unset (legacy charts)', () => {
+  expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
+});
+
+test('keeps null values when showNullValues is true', () => {
+  expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
+});
+
+// Single-column sunburst: the toggle must actually drop the null node.
+test('removes null values when showNullValues is false', () => {
+  expect(seriesValues(nullValueProps(false))).toEqual([10, 20]);
+});

superset-frontend/plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts

@@ -394,7 +394,7 @@ describe('test treeBuilder', () => {
     ]);
   });
 
-  test('filter null values', () => {
+  test('filter null values in a nested layer (parent total excludes hidden nulls)', () => {
     const tree = treeBuilder(
       [
         ...data,
@@ -426,6 +426,8 @@ describe('test treeBuilder', () => {
         value: 2,
       },
       {
+        // The null `bar` child is removed AND its value is excluded from the
+        // parent total, so the arc stays sized to its visible children (no gap).
         children: [
           {
             groupBy: 'bar',
@@ -436,8 +438,8 @@ describe('test treeBuilder', () => {
         ],
         groupBy: 'foo',
         name: 'a-2',
-        secondaryValue: 4,
-        value: 4,
+        secondaryValue: 2,
+        value: 2,
       },
       {
         children: [
@@ -511,4 +513,137 @@ describe('test treeBuilder', () => {
       },
     ]);
   });
+
+  // Regression: a single-level (single column) sunburst previously never
+  // filtered, because filtering only happened in the multi-level branch.
+  test('single-level: shows null nodes when filtering is off', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'a', count: 2, count2: 3 },
+        { foo: null, count: 5, count2: 7 },
+      ],
+      ['foo'],
+      'count',
+    );
+    expect(tree).toEqual([
+      { groupBy: 'foo', name: 'a', secondaryValue: 2, value: 2 },
+      { groupBy: 'foo', name: null, secondaryValue: 5, value: 5 },
+    ]);
+  });
+
+  test('single-level: removes null nodes when filtering is on', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'a', count: 2, count2: 3 },
+        { foo: null, count: 5, count2: 7 },
+      ],
+      ['foo'],
+      'count',
+      undefined,
+      true,
+    );
+    expect(tree).toEqual([
+      { groupBy: 'foo', name: 'a', secondaryValue: 2, value: 2 },
+    ]);
+  });
+
+  // Regression: a null in the *root* (first) column previously slipped through
+  // because the top-level result array was never filtered.
+  test('multi-level: shows null root node when filtering is off', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'a-1', bar: 'a', count: 2, count2: 3 },
+        { foo: null, bar: 'x', count: 5, count2: 7 },
+      ],
+      ['foo', 'bar'],
+      'count',
+    );
+    expect(tree).toEqual([
+      {
+        children: [{ groupBy: 'bar', name: 'a', secondaryValue: 2, value: 2 }],
+        groupBy: 'foo',
+        name: 'a-1',
+        secondaryValue: 2,
+        value: 2,
+      },
+      {
+        children: [{ groupBy: 'bar', name: 'x', secondaryValue: 5, value: 5 }],
+        groupBy: 'foo',
+        name: null,
+        secondaryValue: 5,
+        value: 5,
+      },
+    ]);
+  });
+
+  test('multi-level: removes null root node (and its subtree) when filtering is on', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'a-1', bar: 'a', count: 2, count2: 3 },
+        { foo: null, bar: 'x', count: 5, count2: 7 },
+      ],
+      ['foo', 'bar'],
+      'count',
+      undefined,
+      true,
+    );
+    expect(tree).toEqual([
+      {
+        children: [{ groupBy: 'bar', name: 'a', secondaryValue: 2, value: 2 }],
+        groupBy: 'foo',
+        name: 'a-1',
+        secondaryValue: 2,
+        value: 2,
+      },
+    ]);
+  });
+
+  // With a secondary metric, the parent's secondaryValue must also exclude the
+  // hidden null child rather than leaving a stale (inflated) total.
+  test('filtering excludes hidden nulls from secondary-metric totals', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'p', bar: 'a', count: 2, count2: 3 },
+        { foo: 'p', bar: null, count: 2, count2: 7 },
+      ],
+      ['foo', 'bar'],
+      'count',
+      'count2',
+      true,
+    );
+    expect(tree).toEqual([
+      {
+        children: [{ groupBy: 'bar', name: 'a', secondaryValue: 3, value: 2 }],
+        groupBy: 'foo',
+        name: 'p',
+        secondaryValue: 3,
+        value: 2,
+      },
+    ]);
+  });
+
+  // A parent whose children are all null must be dropped, not kept as a
+  // zero-value arc: a retained `value: 0` node yields NaN for the
+  // secondaryValue/value ratio used in linear coloring and tooltips.
+  test('filtering drops parents left with no children', () => {
+    const tree = treeBuilder(
+      [
+        { foo: 'keep', bar: 'a', count: 2, count2: 3 },
+        { foo: 'drop', bar: null, count: 5, count2: 7 },
+      ],
+      ['foo', 'bar'],
+      'count',
+      'count2',
+      true,
+    );
+    expect(tree).toEqual([
+      {
+        children: [{ groupBy: 'bar', name: 'a', secondaryValue: 3, value: 2 }],
+        groupBy: 'foo',
+        name: 'keep',
+        secondaryValue: 3,
+        value: 2,
+      },
+    ]);
+  });
 });

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx

@@ -32,11 +32,7 @@ fetchMock.put('glob:*/api/v1/dashboard/*/colors*', {});
 fetchMock.post('glob:*/superset/log/?*', {});
 
 jest.mock('@visx/responsive', () => ({
-  ParentSize: ({
-    children,
-  }: {
-    children: (props: { width: number }) => JSX.Element;
-  }) => children({ width: 800 }),
+  useParentSize: () => ({ parentRef: { current: null }, width: 800 }),
 }));
 
 jest.mock('src/dashboard/containers/DashboardGrid', () => ({

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx

@@ -38,7 +38,7 @@ import {
   NativeFilterType,
   getLabelsColorMap,
 } from '@superset-ui/core';
-import { ParentSize } from '@visx/responsive';
+import { useParentSize } from '@visx/responsive';
 import Tabs from '@superset-ui/core/components/Tabs';
 import DashboardGrid from 'src/dashboard/containers/DashboardGrid';
 import {
@@ -374,9 +374,13 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
     [activeKey, childIds, dashboardLayout, handleFocus, renderTabBar, tabIndex],
   );
 
+  // Hook form, not <ParentSize>: @visx 4.0.0's component clips content taller
+  // than the viewport, which breaks dashboard page scrolling.
+  const { parentRef, width } = useParentSize();
+
   return (
-    <div className="grid-container" data-test="grid-container">
-      <ParentSize>{renderParentSizeChildren}</ParentSize>
+    <div className="grid-container" data-test="grid-container" ref={parentRef}>
+      {renderParentSizeChildren({ width })}
     </div>
   );
 };

superset-frontend/src/features/databases/DatabaseModal/index.test.tsx

@@ -77,18 +77,22 @@ const databaseFixture: DatabaseObject = {
 // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
 describe('DatabaseModal', () => {
   beforeEach(() => {
-    fetchMock.post(DATABASE_CONNECT_ENDPOINT, {
-      id: 10,
-      result: {
-        configuration_method: 'sqlalchemy_form',
-        database_name: 'Other2',
-        driver: 'apsw',
-        expose_in_sqllab: true,
-        extra: '{"allows_virtual_table_explore":true}',
-        sqlalchemy_uri: 'gsheets://',
+    fetchMock.post(
+      DATABASE_CONNECT_ENDPOINT,
+      {
+        id: 10,
+        result: {
+          configuration_method: 'sqlalchemy_form',
+          database_name: 'Other2',
+          driver: 'apsw',
+          expose_in_sqllab: true,
+          extra: '{"allows_virtual_table_explore":true}',
+          sqlalchemy_uri: 'gsheets://',
+        },
+        json: 'foo',
       },
-      json: 'foo',
-    });
+      { name: 'database-connect' },
+    );
 
     fetchMock.get(DATABASE_FETCH_ENDPOINT, {
       result: {
@@ -311,9 +315,13 @@ describe('DatabaseModal', () => {
         },
       ],
     });
-    fetchMock.post(VALIDATE_PARAMS_ENDPOINT, {
-      message: 'OK',
-    });
+    fetchMock.post(
+      VALIDATE_PARAMS_ENDPOINT,
+      {
+        message: 'OK',
+      },
+      { name: 'validate-params' },
+    );
   });
 
   beforeEach(() => {
@@ -1367,14 +1375,16 @@ describe('DatabaseModal', () => {
         const textboxes = screen.getAllByRole('textbox');
         const hostField = textboxes[0];
         const portField = screen.getByRole('spinbutton');
-        const databaseNameField = textboxes[1];
+        // textboxes[1] is the connection `database` field; the engine display
+        // name (`database_name`) auto-fills and is asserted separately.
+        const databaseField = textboxes[1];
         const usernameField = textboxes[2];
         const passwordField = textboxes[3];
         const connectButton = screen.getByRole('button', { name: 'Connect' });
 
         expect(hostField).toHaveValue('');
         expect(portField).toHaveValue(null);
-        expect(databaseNameField).toHaveValue('');
+        expect(databaseField).toHaveValue('');
         expect(usernameField).toHaveValue('');
         expect(passwordField).toHaveValue('');
 
@@ -1382,7 +1392,7 @@ describe('DatabaseModal', () => {
 
         userEvent.type(hostField, 'localhost');
         userEvent.type(portField, '5432');
-        userEvent.type(databaseNameField, 'postgres');
+        userEvent.type(databaseField, 'postgres');
         userEvent.type(usernameField, 'testdb');
         userEvent.type(passwordField, 'demoPassword');
 
@@ -1391,7 +1401,7 @@ describe('DatabaseModal', () => {
         expect(await screen.findByDisplayValue(/5432/i)).toBeInTheDocument();
         expect(hostField).toHaveValue('localhost');
         expect(portField).toHaveValue(5432);
-        expect(databaseNameField).toHaveValue('postgres');
+        expect(databaseField).toHaveValue('postgres');
         expect(usernameField).toHaveValue('testdb');
         expect(passwordField).toHaveValue('demoPassword');
 
@@ -1581,6 +1591,135 @@ describe('DatabaseModal', () => {
       ).toBeInTheDocument();
     });
   });
+
+  // Tests migrated from the deprecated Cypress suite
+  // (cypress-base/cypress/e2e/database/modal.test.ts). The two "error alert"
+  // cases originally relied on a real backend connection attempt (real DNS /
+  // socket behaviour), which made them flaky. They are reproduced here by
+  // mocking the validate_parameters response: the frontend's responsibility is
+  // to map an `extra.invalid` field error onto the matching form field, which
+  // is exactly what these assertions exercise. Whether a bad host/port really
+  // fails to connect is a backend concern, covered by backend tests.
+  const selectPostgres = async () => {
+    userEvent.click(await screen.findByRole('button', { name: /postgresql/i }));
+    // Dynamic form (step 2 of 3) is now visible
+    expect(await screen.findByText(/step 2 of 3/i)).toBeInTheDocument();
+  };
+
+  // The modal renders into a portal on document.body, so fields are queried
+  // from the document rather than the render container.
+  const fieldByName = (name: string) =>
+    document.querySelector(`input[name="${name}"]`) as HTMLInputElement;
+
+  const fillDynamicForm = () => {
+    const values: Record<string, string> = {
+      host: 'badhost',
+      port: '5432',
+      database: 'testdb',
+      username: 'testusername',
+      password: 'testpass',
+    };
+    Object.entries(values).forEach(([name, value]) =>
+      userEvent.type(fieldByName(name), value),
+    );
+  };
+
+  test('defaults the display name to the selected engine', async () => {
+    setup();
+    await selectPostgres();
+
+    // The display name field auto-fills with the selected engine's name. The
+    // empty initial state of the connection fields (host/port/database/…) is
+    // already covered by the "enters form credentials" test above.
+    expect(fieldByName('database_name')).toHaveValue('PostgreSQL');
+  });
+
+  test('switches to the SQLAlchemy URI form via the connect link', async () => {
+    setup();
+    await selectPostgres();
+
+    userEvent.click(screen.getByTestId('sqla-connect-btn'));
+
+    expect(await screen.findByTestId('database-name-input')).toBeVisible();
+    expect(screen.getByTestId('sqlalchemy-uri-input')).toBeVisible();
+  });
+
+  test.each([
+    {
+      field: 'host',
+      errorType: 'CONNECTION_INVALID_HOSTNAME_ERROR',
+      message: "The hostname provided can't be resolved.",
+    },
+    {
+      field: 'port',
+      errorType: 'CONNECTION_PORT_CLOSED_ERROR',
+      message: 'The port is closed.',
+    },
+  ])(
+    'surfaces a $field validation error returned by validate_parameters',
+    async ({ field, errorType, message }) => {
+      const createResource = jest.fn();
+      const useSingleViewResourceMock = jest
+        .spyOn(hooks, 'useSingleViewResource')
+        .mockReturnValue({
+          state: {
+            loading: false,
+            resource: null,
+            error: null,
+          },
+          fetchResource: jest.fn(),
+          createResource,
+          updateResource: jest.fn(),
+          clearError: jest.fn(),
+          setResource: jest.fn(),
+        });
+
+      setup();
+      await selectPostgres();
+      fillDynamicForm();
+
+      const submitButton = screen.getByTestId('btn-submit-connection');
+      // Blur the last field and let the (default, passing) validation settle
+      // so its async onBlur result can't race with — and overwrite — the
+      // submit-time validation below. Mirrors the Cypress `body.click(0, 0)`.
+      userEvent.click(document.body);
+      await waitFor(() => expect(submitButton).toBeEnabled());
+
+      // Make validation fail the way a real backend would for an unreachable
+      // host / closed port. The frontend's job is to map `extra.invalid:
+      // [field]` onto the matching form field. The 422 short-circuits the
+      // submit before the create (database-connect) request fires, so only
+      // this validate_parameters mock drives the rendered error.
+      fetchMock.modifyRoute('validate-params', {
+        response: {
+          status: 422,
+          body: {
+            errors: [
+              {
+                message,
+                error_type: errorType,
+                level: 'error',
+                extra: { invalid: [field] },
+              },
+            ],
+          },
+        },
+      });
+
+      userEvent.click(submitButton);
+
+      // Wait for the async error to render, then confirm it surfaced as an
+      // antd inline field error (not a general alert)...
+      const errorText = await screen.findByText(message);
+      expect(errorText.closest('.ant-form-item-explain-error')).not.toBeNull();
+      // ...and that it belongs to the form item for the field named in
+      // `extra.invalid` — i.e. that input lives in the same `.ant-form-item`.
+      const formItem = errorText.closest('.ant-form-item') as HTMLElement;
+      expect(formItem.querySelector(`input[name="${field}"]`)).not.toBeNull();
+      expect(createResource).not.toHaveBeenCalled();
+      useSingleViewResourceMock.mockRestore();
+    },
+  );
 });
 
 test('handleChangeWithValidation function clears validation errors when called', () => {

superset-frontend/src/features/databases/DatabaseModal/index.tsx

@@ -940,7 +940,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       }
 
       const errors = await getValidation(dbToUpdate, true);
-      if (!isEmpty(validationErrors) || errors?.length) {
+      if (!isEmpty(validationErrors) || !isEmpty(errors)) {
         addDangerToast(
           t('Connection failed, please check your connection settings.'),
         );

superset-frontend/src/views/App.tsx

@@ -123,13 +123,24 @@ const contentCss = css`
   position: relative;
 `;
 
-/**
- * Renders the main content area. When the chat panel is open in panel mode,
- * wraps <Layout> and <ChatPanelContent> in a Splitter so they sit side-by-side
- * with a lazy drag bar (blue preview line, resize committed on mouseup).
- * The full <Layout> tree lives inside the first panel so its internal flex
- * context is preserved — SQL Lab, Explore, and other pages are unaffected.
- */
+// Chat panel mode locks the shell (content scrolls inside its panel); every
+// other state page-scrolls so the navbar hides on scroll.
+const lockedShellCss = css`
+  height: 100vh;
+  overflow: hidden;
+`;
+
+const pageScrollShellCss = css`
+  min-height: 100vh;
+`;
+
+const pageScrollContentCss = css`
+  display: flex;
+  flex-direction: column;
+`;
+
+// Renders the app shell and picks the scroll model: in chat panel mode <Layout>
+// sits in a Splitter beside the chat panel; otherwise the page scrolls normally.
 const AppContent = () => {
   const isAuthenticated =
     isUser(bootstrapData.user) && !bootstrapData.user.isAnonymous;
@@ -145,23 +156,14 @@ const AppContent = () => {
   );
 
   const layoutContent = (
-    <Layout css={layoutCss}>
-      <Layout.Content css={contentCss}>
+    <Layout css={isPanelOpen ? layoutCss : undefined}>
+      <Layout.Content css={isPanelOpen ? contentCss : pageScrollContentCss}>
         <RouteSwitch />
       </Layout.Content>
     </Layout>
   );
 
-  if (!isPanelOpen) {
-    return (
-      <>
-        {layoutContent}
-        {hasChatExtension && <ChatFloatingHost />}
-      </>
-    );
-  }
-
-  return (
+  const content = isPanelOpen ? (
     <Splitter
       lazy
       onResizeEnd={sizes => {
@@ -194,6 +196,21 @@ const AppContent = () => {
         <ChatPanelHost />
       </Splitter.Panel>
     </Splitter>
+  ) : (
+    <>
+      {layoutContent}
+      {hasChatExtension && <ChatFloatingHost />}
+    </>
+  );
+
+  return (
+    <Flex vertical css={isPanelOpen ? lockedShellCss : pageScrollShellCss}>
+      <Menu
+        data={bootstrapData.common.menu_data}
+        isFrontendRoute={isFrontendRoute}
+      />
+      <ExtensionsStartup>{content}</ExtensionsStartup>
+    </Flex>
   );
 };
 
@@ -202,21 +219,7 @@ const App = () => (
     <ScrollToTop />
     <LocationPathnameLogger />
     <RootContextProviders>
-      <Flex
-        vertical
-        css={css`
-          height: 100vh;
-          overflow: hidden;
-        `}
-      >
-        <Menu
-          data={bootstrapData.common.menu_data}
-          isFrontendRoute={isFrontendRoute}
-        />
-        <ExtensionsStartup>
-          <AppContent />
-        </ExtensionsStartup>
-      </Flex>
+      <AppContent />
       <ToastContainer />
     </RootContextProviders>
   </Router>

superset/commands/distributed_lock/acquire.py

@@ -19,11 +19,10 @@ from __future__ import annotations
 
 import logging
 from datetime import datetime, timedelta, timezone
-from functools import partial
 from typing import Any
 
 import redis
-from sqlalchemy.exc import SQLAlchemyError
+from sqlalchemy.exc import IntegrityError, SQLAlchemyError
 
 from superset.commands.distributed_lock.base import (
     BaseDistributedLockCommand,
@@ -31,17 +30,32 @@ from superset.commands.distributed_lock.base import (
     get_redis_client,
 )
 from superset.daos.key_value import KeyValueDAO
-from superset.exceptions import AcquireDistributedLockFailedException
+from superset.exceptions import (
+    AcquireDistributedLockFailedException,
+    LockAlreadyHeldException,
+)
 from superset.key_value.exceptions import (
     KeyValueCodecEncodeException,
     KeyValueUpsertFailedError,
 )
 from superset.key_value.types import KeyValueResource
-from superset.utils.decorators import on_error, transaction
+from superset.utils.decorators import transaction
 
 logger = logging.getLogger(__name__)
 
 
+def _kv_lock_on_error(ex: Exception) -> None:
+    """Translate KV-backend exceptions into the appropriate lock exception type."""
+    if isinstance(ex, IntegrityError):
+        # Unique-constraint violation: lock is already held by another process.
+        raise LockAlreadyHeldException("KV lock already held") from ex
+    if isinstance(
+        ex, (KeyValueCodecEncodeException, KeyValueUpsertFailedError, SQLAlchemyError)
+    ):
+        raise AcquireDistributedLockFailedException(f"KV lock failed: {ex}") from ex
+    raise ex
+
+
 class AcquireDistributedLock(BaseDistributedLockCommand):
     """
     Acquire a distributed lock with automatic backend selection.
@@ -85,7 +99,7 @@ class AcquireDistributedLock(BaseDistributedLockCommand):
 
             if not acquired:
                 logger.debug("Redis lock on %s already taken", self.redis_lock_key)
-                raise AcquireDistributedLockFailedException("Lock already taken")
+                raise LockAlreadyHeldException("Lock already taken")
 
             logger.debug(
                 "Acquired Redis lock: %s (TTL=%ds)",
@@ -99,17 +113,7 @@ class AcquireDistributedLock(BaseDistributedLockCommand):
                 f"Redis lock failed: {ex}"
             ) from ex
 
-    @transaction(
-        on_error=partial(
-            on_error,
-            catches=(
-                KeyValueCodecEncodeException,
-                KeyValueUpsertFailedError,
-                SQLAlchemyError,
-            ),
-            reraise=AcquireDistributedLockFailedException,
-        ),
-    )
+    @transaction(on_error=_kv_lock_on_error)
     def _acquire_kv(self) -> None:
         """Acquire lock using KeyValue table (database)."""
         # Delete expired entries first to prevent stale locks from blocking

superset/config.py

@@ -1183,6 +1183,9 @@ THUMBNAIL_CACHE_CONFIG: CacheConfig = {
     "CACHE_NO_NULL_WARNING": True,
 }
 THUMBNAIL_ERROR_CACHE_TTL = int(timedelta(days=1).total_seconds())
+# How long to treat a COMPUTING cache entry as an active lease before considering
+# the worker stuck.  Should exceed the task soft_time_limit (300 s) by a margin.
+THUMBNAIL_COMPUTING_CACHE_TTL = int(timedelta(seconds=360).total_seconds())
 
 # Cache warmup user — must be set explicitly before enabling the cache-warmup
 # Celery task. Intentionally defaults to None so operators pick a dedicated

superset/dashboards/api.py

@@ -1475,7 +1475,6 @@ class DashboardRestApi(CustomTagsOptimizationMixin, BaseSupersetModelRestApi):
 
         if cache_payload.should_trigger_task(force):
             logger.info("Triggering screenshot ASYNC")
-            screenshot_obj.cache.set(cache_key, ScreenshotCachePayload().to_dict())
             cache_dashboard_screenshot.delay(
                 username=get_current_user(),
                 guest_token=(
@@ -1671,7 +1670,6 @@ class DashboardRestApi(CustomTagsOptimizationMixin, BaseSupersetModelRestApi):
                 "Triggering thumbnail compute (dashboard id: %s) ASYNC",
                 str(dashboard.id),
             )
-            screenshot_obj.cache.set(cache_key, ScreenshotCachePayload().to_dict())
             cache_dashboard_thumbnail.delay(
                 current_user=current_user,
                 dashboard_id=dashboard.id,

superset/distributed_lock/__init__.py

@@ -46,8 +46,10 @@ def DistributedLock(  # noqa: N802
                         to prevent deadlocks from crashed processes.
     :param kwargs: Additional key parameters to differentiate locks
     :yields: UUID identifying this lock acquisition
-    :raises AcquireDistributedLockFailedException: If lock is already held
-            or Redis connection fails
+    :raises LockAlreadyHeldException: If the lock is already held by another process
+            (subclass of AcquireDistributedLockFailedException)
+    :raises AcquireDistributedLockFailedException: If the lock backend fails
+            (Redis/DB connection error)
     """
     # pylint: disable=import-outside-toplevel
     from superset.commands.distributed_lock.acquire import AcquireDistributedLock

superset/exceptions.py

@@ -441,6 +441,14 @@ class AcquireDistributedLockFailedException(Exception):  # noqa: N818
     """
 
 
+class LockAlreadyHeldException(AcquireDistributedLockFailedException):  # noqa: N818
+    """
+    Raised when a distributed lock is already held by another process (lock contention).
+    Subclass of AcquireDistributedLockFailedException so existing callers that catch
+    the base exception continue to work unchanged.
+    """
+
+
 class ReleaseDistributedLockFailedException(Exception):  # noqa: N818
     """
     Exception to signalize failure to release lock.

superset/tasks/thumbnails.py

@@ -27,7 +27,10 @@ from superset.extensions import celery_app
 from superset.security.guest_token import GuestToken
 from superset.tasks.utils import get_executor
 from superset.utils.core import override_user
-from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot
+from superset.utils.screenshots import (
+    ChartScreenshot,
+    DashboardScreenshot,
+)
 from superset.utils.urls import get_url_path
 from superset.utils.webdriver import WindowSize
 
@@ -99,12 +102,15 @@ def cache_dashboard_thumbnail(
     user = security_manager.find_user(username)
     with override_user(user):
         screenshot = DashboardScreenshot(url, dashboard.digest)
+        resolved_cache_key = cache_key or screenshot.get_cache_key(
+            window_size, thumb_size
+        )
         screenshot.compute_and_cache(
             user=user,
             window_size=window_size,
             thumb_size=thumb_size,
             force=force,
-            cache_key=cache_key,
+            cache_key=resolved_cache_key,
         )
 
 

superset/utils/screenshots.py

@@ -26,7 +26,11 @@ from typing import cast, TYPE_CHECKING, TypedDict
 from flask import current_app as app
 
 from superset import feature_flag_manager, thumbnail_cache
-from superset.exceptions import ScreenshotImageNotAvailableException
+from superset.distributed_lock import DistributedLock
+from superset.exceptions import (
+    LockAlreadyHeldException,
+    ScreenshotImageNotAvailableException,
+)
 from superset.extensions import event_logger
 from superset.utils.hashing import hash_from_dict
 from superset.utils.urls import modify_url_query
@@ -119,7 +123,6 @@ class ScreenshotCachePayload:
 
     def computing(self) -> None:
         self.update_timestamp()
-        self._image = None
         self.status = StatusValues.COMPUTING
 
     def update(self, image: bytes) -> None:
@@ -152,9 +155,7 @@ class ScreenshotCachePayload:
 
     def is_computing_stale(self) -> bool:
         """Check if a COMPUTING status is stale (task likely failed or stuck)."""
-        # Use the same TTL as error cache - if computing takes longer than this,
-        # it's likely stuck and should be retried
-        computing_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+        computing_ttl = app.config["THUMBNAIL_COMPUTING_CACHE_TTL"]
         return (
             datetime.now() - datetime.fromisoformat(self.get_timestamp())
         ).total_seconds() >= computing_ttl
@@ -280,43 +281,71 @@ class BaseScreenshot:
         :return: Image payload
         """
         cache_key = cache_key or self.get_cache_key(window_size, thumb_size)
-        cache_payload = self.get_from_cache_key(cache_key) or ScreenshotCachePayload()
-        if not cache_payload.should_trigger_task(force=force):
-            logger.info(
-                "Skipping compute - already processed for thumbnail: %s", cache_key
-            )
-            return
-
-        window_size = window_size or self.window_size
-        thumb_size = thumb_size or self.thumb_size
-        logger.info("Processing url for thumbnail: %s", cache_key)
-        cache_payload.computing()
-        image = None
-        # Assuming all sorts of things can go wrong with Selenium
         try:
-            logger.info("trying to generate screenshot")
-            with event_logger.log_context(f"screenshot.compute.{self.thumbnail_type}"):
-                image = self.get_screenshot(user=user, window_size=window_size)
-        except Exception as ex:  # pylint: disable=broad-except
-            logger.warning("Failed at generating thumbnail %s", ex, exc_info=True)
-            cache_payload.error()
-        if image and window_size != thumb_size:
-            try:
-                image = self.resize_image(image, thumb_size=thumb_size)
-            except Exception as ex:  # pylint: disable=broad-except
-                logger.warning("Failed at resizing thumbnail %s", ex, exc_info=True)
-                cache_payload.error()
+            with DistributedLock(
+                namespace="thumbnail",
+                key=cache_key,
+                ttl_seconds=app.config["THUMBNAIL_COMPUTING_CACHE_TTL"],
+            ):
+                cache_payload = (
+                    self.get_from_cache_key(cache_key) or ScreenshotCachePayload()
+                )
+                if not cache_payload.should_trigger_task(force=force):
+                    logger.info(
+                        "Skipping compute - already processed for thumbnail: %s",
+                        cache_key,
+                    )
+                    return
+
+                window_size = window_size or self.window_size
+                thumb_size = thumb_size or self.thumb_size
+                logger.info("Processing url for thumbnail: %s", cache_key)
+                cache_payload.computing()
+                self.cache.set(cache_key, cache_payload.to_dict())
                 image = None
+                # Assuming all sorts of things can go wrong with Selenium
+                try:
+                    logger.info("trying to generate screenshot")
+                    with event_logger.log_context(
+                        f"screenshot.compute.{self.thumbnail_type}"
+                    ):
+                        image = self.get_screenshot(user=user, window_size=window_size)
+                except Exception as ex:  # pylint: disable=broad-except
+                    logger.warning(
+                        "Failed at generating thumbnail %s", ex, exc_info=True
+                    )
+                    cache_payload.error()
+                if image and window_size != thumb_size:
+                    try:
+                        image = self.resize_image(image, thumb_size=thumb_size)
+                    except Exception as ex:  # pylint: disable=broad-except
+                        logger.warning(
+                            "Failed at resizing thumbnail %s", ex, exc_info=True
+                        )
+                        cache_payload.error()
+                        image = None
 
-        # Cache the result (success or error) to avoid immediate retries
-        if image:
-            with event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
-                cache_payload.update(image)
+                # Cache the result (success or error) to avoid immediate retries
+                if image:
+                    with event_logger.log_context(
+                        f"screenshot.cache.{self.thumbnail_type}"
+                    ):
+                        cache_payload.update(image)
+                elif cache_payload.status != StatusValues.ERROR:
+                    # Only call error() if not already set — avoids overwriting
+                    # the timestamp recorded when the actual failure occurred above.
+                    cache_payload.error()
 
-        logger.info("Caching thumbnail: %s", cache_key)
-        self.cache.set(cache_key, cache_payload.to_dict())
-        logger.info("Updated thumbnail cache; Status: %s", cache_payload.get_status())
-        return
+                logger.info("Caching thumbnail: %s", cache_key)
+                self.cache.set(cache_key, cache_payload.to_dict())
+                logger.info(
+                    "Updated thumbnail cache; Status: %s", cache_payload.get_status()
+                )
+        except LockAlreadyHeldException:
+            logger.info(
+                "Skipping duplicate thumbnail task for %s - lock already held",
+                cache_key,
+            )
 
     @classmethod
     def resize_image(

superset/utils/webdriver.py

@@ -342,15 +342,6 @@ class WebDriverPlaywright(WebDriverProxy):
                 selenium_animation_wait = app.config[
                     "SCREENSHOT_SELENIUM_ANIMATION_WAIT"
                 ]
-                logger.debug(
-                    "Wait %i seconds for chart animation", selenium_animation_wait
-                )
-                page.wait_for_timeout(selenium_animation_wait * 1000)
-                logger.debug(
-                    "Taking a PNG screenshot of url %s as user %s",
-                    url,
-                    user.username if user else "None",
-                )
                 if app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]:
                     unexpected_errors = WebDriverPlaywright.find_unexpected_errors(page)
                     if unexpected_errors:
@@ -415,21 +406,38 @@ class WebDriverPlaywright(WebDriverProxy):
                             load_wait=self._screenshot_load_wait,
                             animation_wait=selenium_animation_wait,
                         )
-                        if img is None:
-                            logger.error(
-                                "Tiled screenshot failed at url %s; "
-                                "not falling back to avoid sending a blank PDF",
-                                url,
+                        if not img:
+                            logger.warning(
+                                (
+                                    "Tiled screenshot failed, "
+                                    "falling back to standard screenshot"
+                                )
                             )
-                            return None
+                            img = WebDriverPlaywright._get_screenshot(
+                                page, element, element_name
+                            )
+                        logger.debug(
+                            "Tiled screenshot result: %d bytes for url: %s",
+                            len(img) if img else 0,
+                            url,
+                        )
                     else:
+                        logger.debug(
+                            "Dashboard below tiling threshold "
+                            "(%s charts, %spx height); using standard screenshot "
+                            "for url: %s",
+                            chart_count,
+                            dashboard_height,
+                            url,
+                        )
                         # Standard screenshot captures the full element including
                         # below-the-fold content, so wait for all spinners globally.
                         try:
                             logger.debug(
-                                "Wait for loading element of charts to be gone"
-                                " at url: %s",
+                                "Waiting for all spinners to clear at url: %s "
+                                "(SCREENSHOT_LOAD_WAIT=%ss)",
                                 url,
+                                self._screenshot_load_wait,
                             )
                             page.wait_for_function(
                                 "() => document.querySelectorAll("
@@ -444,16 +452,40 @@ class WebDriverPlaywright(WebDriverProxy):
                                 self._screenshot_load_wait,
                             )
                             raise
+                        logger.debug("All spinners cleared for url: %s", url)
+                        if selenium_animation_wait > 0:
+                            logger.debug(
+                                "Wait %i seconds for chart animation",
+                                selenium_animation_wait,
+                            )
+                            page.wait_for_timeout(selenium_animation_wait * 1000)
+                        logger.debug(
+                            "Taking screenshot of url %s as user %s",
+                            url,
+                            user.username if user else "None",
+                        )
                         img = WebDriverPlaywright._get_screenshot(
                             page, element, element_name
                         )
+                        logger.debug(
+                            "Screenshot result: %d bytes for url: %s",
+                            len(img) if img else 0,
+                            url,
+                        )
                 else:
+                    logger.debug(
+                        "Tiled screenshots disabled; using standard screenshot "
+                        "for url: %s",
+                        url,
+                    )
                     # Standard screenshot captures the full element including
                     # below-the-fold content, so wait for all spinners globally.
                     try:
                         logger.debug(
-                            "Wait for loading element of charts to be gone at url: %s",
+                            "Waiting for all spinners to clear at url: %s "
+                            "(SCREENSHOT_LOAD_WAIT=%ss)",
                             url,
+                            self._screenshot_load_wait,
                         )
                         page.wait_for_function(
                             "() => document.querySelectorAll('.loading').length === 0",
@@ -467,9 +499,26 @@ class WebDriverPlaywright(WebDriverProxy):
                             self._screenshot_load_wait,
                         )
                         raise
+                    logger.debug("All spinners cleared for url: %s", url)
+                    if selenium_animation_wait > 0:
+                        logger.debug(
+                            "Wait %i seconds for chart animation",
+                            selenium_animation_wait,
+                        )
+                        page.wait_for_timeout(selenium_animation_wait * 1000)
+                    logger.debug(
+                        "Taking screenshot of url %s as user %s",
+                        url,
+                        user.username if user else "None",
+                    )
                     img = WebDriverPlaywright._get_screenshot(
                         page, element, element_name
                     )
+                    logger.debug(
+                        "Screenshot result: %d bytes for url: %s",
+                        len(img) if img else 0,
+                        url,
+                    )
 
             except PlaywrightTimeout:
                 raise

tests/unit_tests/tasks/test_thumbnails.py

@@ -0,0 +1,145 @@
+# 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.
+
+from collections.abc import Callable, Iterator
+from typing import Any
+from unittest.mock import ANY, MagicMock, patch
+
+import pytest
+
+
+@pytest.fixture
+def mock_thumbnail_cache() -> Iterator[None]:
+    """Enable thumbnail cache mocking so tasks don't exit early."""
+    with patch("superset.tasks.thumbnails.thumbnail_cache", True):
+        yield
+
+
+@pytest.fixture
+def mock_dashboard() -> MagicMock:
+    """Return a mocked Dashboard with id=1 and a fixed digest."""
+    dashboard = MagicMock()
+    dashboard.id = 1
+    dashboard.digest = "test_digest"
+    return dashboard
+
+
+def _make_screenshot_mock(cache_key: str = "test_cache_key") -> MagicMock:
+    """Return a MagicMock screenshot whose get_cache_key returns cache_key."""
+    screenshot = MagicMock()
+    screenshot.get_cache_key.return_value = cache_key
+    return screenshot
+
+
+# ---------------------------------------------------------------------------
+# Helpers shared across tests
+# ---------------------------------------------------------------------------
+
+_COMMON_PATCHES = [
+    "superset.tasks.thumbnails.get_executor",
+    "superset.tasks.thumbnails.security_manager",
+    "superset.tasks.thumbnails.override_user",
+    "superset.tasks.thumbnails.get_url_path",
+    "superset.tasks.thumbnails.DashboardScreenshot",
+]
+
+
+def _apply_patches(fn: Callable[..., Any]) -> Callable[..., Any]:
+    """Stack the five common patches onto a test function."""
+    for p in reversed(_COMMON_PATCHES):
+        fn = patch(p)(fn)
+    return fn
+
+
+# ---------------------------------------------------------------------------
+# Tests
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.tasks.thumbnails.thumbnail_cache", None)
+def test_cache_dashboard_thumbnail_skips_when_no_cache() -> None:
+    """Task exits early when no thumbnail cache is configured."""
+    from superset.tasks.thumbnails import cache_dashboard_thumbnail
+
+    result = cache_dashboard_thumbnail(current_user="admin", dashboard_id=1, force=True)
+
+    assert result is None
+
+
+@_apply_patches
+def test_cache_dashboard_thumbnail_always_delegates_to_compute_and_cache(
+    mock_screenshot_cls: MagicMock,
+    mock_get_url_path: MagicMock,
+    mock_override_user: MagicMock,
+    mock_security_manager: MagicMock,
+    mock_get_executor: MagicMock,
+    mock_dashboard: MagicMock,
+    mock_thumbnail_cache: None,
+) -> None:
+    """Task always calls compute_and_cache; dedup lives inside compute_and_cache."""
+    from superset.tasks.thumbnails import cache_dashboard_thumbnail
+
+    mock_screenshot = _make_screenshot_mock()
+    mock_screenshot_cls.return_value = mock_screenshot
+    mock_get_executor.return_value = (None, "admin")
+    mock_security_manager.find_user.return_value = MagicMock()
+
+    with patch("superset.models.dashboard.Dashboard.get", return_value=mock_dashboard):
+        cache_dashboard_thumbnail(
+            current_user="admin",
+            dashboard_id=1,
+            force=False,
+            cache_key="test_cache_key",
+        )
+
+    mock_screenshot.compute_and_cache.assert_called_once()
+
+
+@_apply_patches
+def test_cache_dashboard_thumbnail_resolves_cache_key_when_not_provided(
+    mock_screenshot_cls: MagicMock,
+    mock_get_url_path: MagicMock,
+    mock_override_user: MagicMock,
+    mock_security_manager: MagicMock,
+    mock_get_executor: MagicMock,
+    mock_dashboard: MagicMock,
+    mock_thumbnail_cache: None,
+) -> None:
+    """Task resolves cache_key from the screenshot object when none is given."""
+    from superset.tasks.thumbnails import cache_dashboard_thumbnail
+
+    mock_screenshot = _make_screenshot_mock("resolved_cache_key")
+    mock_screenshot_cls.return_value = mock_screenshot
+    mock_get_executor.return_value = (None, "admin")
+    mock_security_manager.find_user.return_value = MagicMock()
+
+    with patch("superset.models.dashboard.Dashboard.get", return_value=mock_dashboard):
+        cache_dashboard_thumbnail(
+            current_user="admin",
+            dashboard_id=1,
+            force=False,
+            cache_key=None,
+        )
+
+    mock_screenshot.get_cache_key.assert_called_once()
+    mock_screenshot.compute_and_cache.assert_called_once_with(
+        user=ANY,
+        window_size=None,
+        thumb_size=None,
+        force=False,
+        cache_key="resolved_cache_key",
+    )

tests/unit_tests/utils/test_screenshot_cache_fix.py

@@ -27,6 +27,7 @@ from unittest.mock import MagicMock, patch
 import pytest
 from pytest_mock import MockerFixture
 
+from superset.exceptions import LockAlreadyHeldException
 from superset.utils.screenshots import (
     BaseScreenshot,
     ScreenshotCachePayload,
@@ -34,6 +35,7 @@ from superset.utils.screenshots import (
 )
 
 BASE_SCREENSHOT_PATH = "superset.utils.screenshots.BaseScreenshot"
+DISTRIBUTED_LOCK_PATH = "superset.utils.screenshots.DistributedLock"
 
 
 class MockCache:
@@ -56,7 +58,7 @@ class MockCache:
 
 
 @pytest.fixture
-def mock_user():
+def mock_user() -> MagicMock:
     """Fixture to create a mock user."""
     user = MagicMock()
     user.id = 1
@@ -64,7 +66,7 @@ def mock_user():
 
 
 @pytest.fixture
-def screenshot_obj():
+def screenshot_obj() -> BaseScreenshot:
     """Fixture to create a BaseScreenshot object."""
     url = "http://example.com"
     digest = "sample_digest"
@@ -74,8 +76,11 @@ def screenshot_obj():
 class TestCacheOnlyOnSuccess:
     """Test that cache is only saved when image generation succeeds."""
 
-    def _setup_mocks(self, mocker: MockerFixture, screenshot_obj):
+    def _setup_mocks(
+        self, mocker: MockerFixture, screenshot_obj: BaseScreenshot
+    ) -> MagicMock:
         """Helper method to set up common mocks."""
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
         mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None)
         get_screenshot = mocker.patch(
             BASE_SCREENSHOT_PATH + ".get_screenshot", return_value=b"image_data"
@@ -91,6 +96,7 @@ class TestCacheOnlyOnSuccess:
         self, mocker: MockerFixture, screenshot_obj, mock_user
     ):
         """Test that error status is cached when screenshot generation fails."""
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
         mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None)
         get_screenshot = mocker.patch(
             BASE_SCREENSHOT_PATH + ".get_screenshot",
@@ -149,38 +155,60 @@ class TestCacheOnlyOnSuccess:
         assert cached_value["status"] == "Updated"
         assert cached_value["image"] is not None
 
-    def test_no_intermediate_cache_during_computing(
-        self, mocker: MockerFixture, screenshot_obj, mock_user
-    ):
-        """Test that cache is not saved during COMPUTING state."""
+    def test_cache_error_status_when_screenshot_returns_empty_bytes(
+        self,
+        mocker: MockerFixture,
+        screenshot_obj: BaseScreenshot,
+        mock_user: MagicMock,
+    ) -> None:
+        """Empty bytes from get_screenshot must set ERROR, not leave COMPUTING."""
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
+        mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None)
+        mocker.patch(
+            BASE_SCREENSHOT_PATH + ".get_screenshot",
+            return_value=b"",
+        )
+        BaseScreenshot.cache = MockCache()
+
+        screenshot_obj.compute_and_cache(user=mock_user, force=True)
+
+        cache_key = screenshot_obj.get_cache_key()
+        cached_value = BaseScreenshot.cache.get(cache_key)
+        assert cached_value is not None
+        assert cached_value["status"] == "Error"
+        assert cached_value.get("image") is None
+
+    def test_computing_status_written_to_cache_early(
+        self,
+        mocker: MockerFixture,
+        screenshot_obj: BaseScreenshot,
+        mock_user: MagicMock,
+    ) -> None:
+        """compute_and_cache writes COMPUTING to cache before taking the screenshot
+        so concurrent tasks can detect it and avoid duplicate work."""
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
         mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None)
         BaseScreenshot.cache = MockCache()
 
-        # Mock get_screenshot to check cache state during execution
-        def check_cache_during_screenshot(*args, **kwargs):
-            # At this point, we're in COMPUTING state
-            # Cache should NOT be set yet
+        def check_cache_during_screenshot(*args: object, **kwargs: object) -> bytes:
             cache_key = screenshot_obj.get_cache_key()
             cached_value = BaseScreenshot.cache.get(cache_key)
-            # Cache should be empty during screenshot generation
-            assert cached_value is None, (
-                "Cache should not be saved during COMPUTING state"
+            assert cached_value is not None, (
+                "Cache should be set to COMPUTING before screenshot starts"
             )
+            assert cached_value["status"] == "Computing"
             return b"image_data"
 
         mocker.patch(
             BASE_SCREENSHOT_PATH + ".get_screenshot",
             side_effect=check_cache_during_screenshot,
         )
-        # Mock resize to avoid PIL errors with fake image data
         mocker.patch(
             BASE_SCREENSHOT_PATH + ".resize_image", return_value=b"resized_image_data"
         )
 
-        # Execute compute_and_cache
         screenshot_obj.compute_and_cache(user=mock_user, force=True)
 
-        # After completion, cache should be set with UPDATED status
         cache_key = screenshot_obj.get_cache_key()
         cached_value = BaseScreenshot.cache.get(cache_key)
         assert cached_value is not None
@@ -191,10 +219,10 @@ class TestShouldTriggerTask:
     """Test the should_trigger_task method improvements."""
 
     @patch("superset.utils.screenshots.app")
-    def test_trigger_on_stale_computing_status(self, mock_app):
+    def test_trigger_on_stale_computing_status(self, mock_app: MagicMock) -> None:
         """Test that stale COMPUTING status triggers recomputation."""
         # Set TTL to 300 seconds
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Create payload with COMPUTING status from 400 seconds ago (stale)
         old_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
@@ -209,7 +237,7 @@ class TestShouldTriggerTask:
     def test_no_trigger_on_fresh_computing_status(self, mock_app):
         """Test that fresh COMPUTING status does not trigger recomputation."""
         # Set TTL to 300 seconds
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Create payload with COMPUTING status from 100 seconds ago (fresh)
         fresh_timestamp = (datetime.now() - timedelta(seconds=100)).isoformat()
@@ -246,8 +274,10 @@ class TestShouldTriggerTask:
     @patch("superset.utils.screenshots.app")
     def test_trigger_on_expired_error(self, mock_app):
         """Test that expired ERROR status triggers task."""
-        # Set TTL to 300 seconds
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {
+            "THUMBNAIL_COMPUTING_CACHE_TTL": 300,
+            "THUMBNAIL_ERROR_CACHE_TTL": 300,
+        }
 
         # Create payload with ERROR status from 400 seconds ago (expired)
         old_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
@@ -258,10 +288,12 @@ class TestShouldTriggerTask:
         assert payload.should_trigger_task(force=False) is True
 
     @patch("superset.utils.screenshots.app")
-    def test_no_trigger_on_fresh_error(self, mock_app):
+    def test_no_trigger_on_fresh_error(self, mock_app: MagicMock) -> None:
         """Test that fresh ERROR status does not trigger task."""
-        # Set TTL to 300 seconds
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {
+            "THUMBNAIL_COMPUTING_CACHE_TTL": 300,
+            "THUMBNAIL_ERROR_CACHE_TTL": 300,
+        }
 
         # Create payload with ERROR status from 100 seconds ago (fresh)
         fresh_timestamp = (datetime.now() - timedelta(seconds=100)).isoformat()
@@ -288,7 +320,7 @@ class TestIsComputingStale:
     @patch("superset.utils.screenshots.app")
     def test_computing_is_stale(self, mock_app):
         """Test that old COMPUTING status is detected as stale."""
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Timestamp from 400 seconds ago
         old_timestamp = (datetime.now() - timedelta(seconds=400)).isoformat()
@@ -301,7 +333,7 @@ class TestIsComputingStale:
     @patch("superset.utils.screenshots.app")
     def test_computing_is_not_stale(self, mock_app):
         """Test that fresh COMPUTING status is not stale."""
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Timestamp from 100 seconds ago
         fresh_timestamp = (datetime.now() - timedelta(seconds=100)).isoformat()
@@ -314,7 +346,7 @@ class TestIsComputingStale:
     @patch("superset.utils.screenshots.app")
     def test_computing_exactly_at_ttl(self, mock_app):
         """Test boundary condition at exactly TTL."""
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Timestamp from exactly 300 seconds ago
         exact_timestamp = (datetime.now() - timedelta(seconds=300)).isoformat()
@@ -328,7 +360,7 @@ class TestIsComputingStale:
     @patch("superset.utils.screenshots.app")
     def test_computing_just_past_ttl(self, mock_app):
         """Test boundary condition just past TTL."""
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
 
         # Timestamp from 301 seconds ago (just past TTL)
         past_ttl_timestamp = (datetime.now() - timedelta(seconds=301)).isoformat()
@@ -350,6 +382,7 @@ class TestIntegrationCacheBugFix:
         Integration test: Failed screenshot should cache error status
         to prevent immediate retries, not leave corrupted cache with image=None.
         """
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
         mocker.patch(
             BASE_SCREENSHOT_PATH + ".get_screenshot",
             side_effect=Exception("Network error"),
@@ -373,13 +406,18 @@ class TestIntegrationCacheBugFix:
 
     @patch("superset.utils.screenshots.app")
     def test_stale_computing_triggers_retry(
-        self, mock_app, mocker: MockerFixture, screenshot_obj, mock_user
-    ):
+        self,
+        mock_app: MagicMock,
+        mocker: MockerFixture,
+        screenshot_obj: BaseScreenshot,
+        mock_user: MagicMock,
+    ) -> None:
         """
         Integration test: Stale COMPUTING status should trigger retry
         to recover from stuck tasks.
         """
-        mock_app.config = {"THUMBNAIL_ERROR_CACHE_TTL": 300}
+        mock_app.config = {"THUMBNAIL_COMPUTING_CACHE_TTL": 300}
+        mocker.patch(DISTRIBUTED_LOCK_PATH)
         BaseScreenshot.cache = MockCache()
 
         # Create stale COMPUTING entry and seed it in the cache
@@ -408,3 +446,39 @@ class TestIntegrationCacheBugFix:
         assert cached_value is not None
         assert cached_value["status"] == "Updated"
         assert cached_value["image"] is not None
+
+    def test_concurrent_task_skips_when_lock_already_held(
+        self,
+        mocker: MockerFixture,
+        screenshot_obj: BaseScreenshot,
+        mock_user: MagicMock,
+    ) -> None:
+        """compute_and_cache exits without rendering when the distributed lock
+        is already held by another worker — atomically preventing duplicate Selenium."""
+        mock_lock = mocker.patch(DISTRIBUTED_LOCK_PATH)
+        mock_lock.return_value.__enter__.side_effect = LockAlreadyHeldException(
+            "lock held"
+        )
+        get_screenshot = mocker.patch(BASE_SCREENSHOT_PATH + ".get_screenshot")
+        BaseScreenshot.cache = MockCache()
+
+        screenshot_obj.compute_and_cache(user=mock_user, force=False)
+
+        get_screenshot.assert_not_called()
+
+    def test_computing_preserves_previous_image(
+        self,
+        mocker: MockerFixture,
+        screenshot_obj: BaseScreenshot,
+        mock_user: MagicMock,
+    ) -> None:
+        """computing() must not wipe the cached image so a stale thumbnail remains
+        visible while a refresh is in progress."""
+        old_image = b"old_thumbnail_bytes"
+        payload = ScreenshotCachePayload(image=old_image)
+        assert payload._image == old_image
+
+        payload.computing()
+
+        assert payload._image == old_image
+        assert payload.status == StatusValues.COMPUTING

tests/unit_tests/utils/webdriver_test.py

@@ -29,7 +29,7 @@ from superset.utils.webdriver import (
 )
 
 
-@pytest.fixture
+@pytest.fixture()
 def mock_app():
     """Mock Flask app with webdriver configuration."""
     app = MagicMock()
@@ -910,11 +910,10 @@ class TestWebDriverPlaywrightErrorHandling:
     @patch("superset.utils.webdriver._browser_manager")
     @patch("superset.utils.webdriver.logger")
     @patch("superset.utils.webdriver.take_tiled_screenshot")
-    def test_tiled_screenshot_failure_returns_none_without_fallback(
+    def test_tiled_screenshot_failure_falls_back_to_standard_screenshot(
         self, mock_take_tiled, mock_logger, mock_browser_manager
     ) -> None:
-        """When take_tiled_screenshot fails, return None rather than fall back to a
-        potentially blank standard screenshot."""
+        """When take_tiled_screenshot returns None, fall back to standard screenshot."""
         mock_user = MagicMock()
         mock_user.username = "test_user"
 
@@ -928,7 +927,8 @@ class TestWebDriverPlaywrightErrorHandling:
         mock_context.new_page.return_value = mock_page
         mock_page.locator.return_value = mock_element
         mock_element.wait_for.return_value = None
-        mock_element.screenshot.return_value = b"should_not_be_called"
+        # page.screenshot is used by _get_screenshot for the "standalone" element
+        mock_page.screenshot.return_value = b"fallback_screenshot"
 
         def evaluate_side_effect(script):
             if "querySelectorAll" in script:
@@ -938,7 +938,7 @@ class TestWebDriverPlaywrightErrorHandling:
             return None
 
         mock_page.evaluate.side_effect = evaluate_side_effect
-        mock_take_tiled.return_value = None  # tiled screenshot fails
+        mock_take_tiled.return_value = None  # tiled screenshot returns None
 
         with patch("superset.utils.webdriver.app") as mock_app:
             mock_app.config = {
@@ -964,13 +964,236 @@ class TestWebDriverPlaywrightErrorHandling:
 
                 driver = WebDriverPlaywright("chrome")
                 result = driver.get_screenshot(
-                    "http://example.com", "dashboard", mock_user
+                    "http://example.com", "standalone", mock_user
                 )
 
-        assert result is None
-        mock_element.screenshot.assert_not_called()
-        mock_logger.error.assert_any_call(
-            "Tiled screenshot failed at url %s; "
-            "not falling back to avoid sending a blank PDF",
-            "http://example.com",
+        assert result == b"fallback_screenshot"
+        mock_take_tiled.assert_called_once()
+        mock_logger.warning.assert_any_call(
+            ("Tiled screenshot failed, falling back to standard screenshot"),
+        )
+
+
+class TestWebDriverPlaywrightAnimationWaitOrder:
+    """Animation wait must run after the spinner wait, not before."""
+
+    _base_config = {
+        "WEBDRIVER_OPTION_ARGS": [],
+        "WEBDRIVER_WINDOW": {"pixel_density": 1},
+        "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
+        "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
+        "SCREENSHOT_SELENIUM_HEADSTART": 0,
+        "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 2,
+        "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
+        "SCREENSHOT_LOCATE_WAIT": 10,
+        "SCREENSHOT_LOAD_WAIT": 30,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+    }
+
+    def _make_pw_mocks(self, mock_browser_manager):
+        mock_browser = MagicMock()
+        mock_context = MagicMock()
+        mock_page = MagicMock()
+        mock_element = MagicMock()
+
+        mock_browser_manager.get_browser.return_value = mock_browser
+        mock_browser.new_context.return_value = mock_context
+        mock_context.new_page.return_value = mock_page
+        mock_page.locator.return_value = mock_element
+        mock_element.screenshot.return_value = b"screenshot"
+        return mock_context, mock_page
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_animation_wait_after_spinner_wait_tiled_disabled(
+        self, mock_app, mock_browser_manager
+    ):
+        """Non-tiled path: animation wait runs after spinner wait_for_function."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        call_order: list[str] = []
+
+        def record_wait_for_function(*args, **kwargs):
+            call_order.append("spinner_wait")
+
+        def record_wait_for_timeout(ms):
+            if ms == 2 * 1000:
+                call_order.append("animation_wait")
+
+        mock_page.wait_for_function.side_effect = record_wait_for_function
+        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        assert "spinner_wait" in call_order
+        assert "animation_wait" in call_order
+        spinner_idx = call_order.index("spinner_wait")
+        anim_idx = call_order.index("animation_wait")
+        assert spinner_idx < anim_idx, (
+            "spinner wait must precede animation wait in non-tiled path"
+        )
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
+        self, mock_app, mock_browser_manager
+    ):
+        """Non-tiled path (tiled on, small dashboard): animation after spinner."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {
+            **self._base_config,
+            "SCREENSHOT_TILED_ENABLED": True,
+            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+        }
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        # Small dashboard: 3 charts, 1000px height — below both thresholds
+        mock_page.evaluate.side_effect = [3, 1000]
+
+        call_order: list[str] = []
+
+        def record_wait_for_function(*args, **kwargs):
+            call_order.append("spinner_wait")
+
+        def record_wait_for_timeout(ms):
+            if ms == 2 * 1000:
+                call_order.append("animation_wait")
+
+        mock_page.wait_for_function.side_effect = record_wait_for_function
+        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        assert "spinner_wait" in call_order
+        assert "animation_wait" in call_order
+        assert call_order.index("spinner_wait") < call_order.index("animation_wait")
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.take_tiled_screenshot")
+    @patch("superset.utils.webdriver.app")
+    def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
+        self, mock_app, mock_take_tiled, mock_browser_manager
+    ):
+        """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {
+            **self._base_config,
+            "SCREENSHOT_TILED_ENABLED": True,
+            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+        }
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        # Large dashboard: 25 charts, 6000px height
+        mock_page.evaluate.side_effect = [25, 6000]
+        mock_take_tiled.return_value = b"tiled_screenshot"
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            result = WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "standalone", mock_user
+            )
+
+        assert result == b"tiled_screenshot"
+        mock_take_tiled.assert_called_once_with(
+            mock_page,
+            "standalone",
+            600,
+            load_wait=30,
+            animation_wait=2,
+        )
+        # The only wait_for_timeout call should be the 0ms headstart; no global
+        # animation wait should be issued (handled per-tile by take_tiled_screenshot)
+        animation_waits = [
+            call[0][0]
+            for call in mock_page.wait_for_timeout.call_args_list
+            if call[0][0] == 2 * 1000
+        ]
+        assert animation_waits == [], (
+            "No global 2s animation wait_for_timeout should fire on the tiled path"
+        )
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.take_tiled_screenshot")
+    @patch("superset.utils.webdriver.app")
+    def test_tiled_fallback_triggered_on_empty_bytes(
+        self, mock_app, mock_take_tiled, mock_browser_manager
+    ):
+        """Tiled fallback fires when take_tiled_screenshot returns b"" (not None)."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {
+            **self._base_config,
+            "SCREENSHOT_TILED_ENABLED": True,
+            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
+            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
+            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
+        }
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+        mock_page.evaluate.side_effect = [25, 6000]
+        # Empty bytes — falsy but not None; was silently passed through before the fix
+        mock_take_tiled.return_value = b""
+        # _get_screenshot("standalone") calls page.screenshot(full_page=True);
+        # configure that return value so we can assert the fallback was reached
+        mock_page.screenshot.return_value = b"fallback"
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            result = WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "standalone", mock_user
+            )
+
+        assert result == b"fallback"
+        # Tiled path was taken (take_tiled_screenshot was called)
+        mock_take_tiled.assert_called_once()
+        # Standard screenshot was called as fallback (full_page=True for "standalone")
+        mock_page.screenshot.assert_called_with(full_page=True)
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_animation_wait_skipped_when_zero(self, mock_app, mock_browser_manager):
+        """No extra wait_for_timeout call when SCREENSHOT_SELENIUM_ANIMATION_WAIT=0."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {
+            **self._base_config,
+            "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0,
+            "SCREENSHOT_TILED_ENABLED": False,
+        }
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            WebDriverPlaywright("chrome").get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        # Only headstart (0ms) should be called; no animation wait call
+        timeout_values = [
+            call[0][0] for call in mock_page.wait_for_timeout.call_args_list
+        ]
+        assert timeout_values == [0], (
+            f"Expected only [0] (headstart), got {timeout_values}"
         )