From 5bb88b2b747b22c025fe8c1ff5ea6a5a9d40c0e1 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 16 Apr 2026 21:42:30 -0700 Subject: [PATCH 1/5] fix(dashboard): restore top-level tab drop target height for dashboards with content PR #37891 moved the DashboardHeader out of the root Droppable, leaving it with zero height when no top-level tabs exist. This made it impossible to drag a Tabs component onto dashboards that already have content. Add min-height to .empty-droptarget in StyledHeader, matching the pattern used by DashboardGrid for its drop targets. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../DashboardBuilder.test.tsx | 42 +++++++++++++++++++ .../DashboardBuilder/DashboardBuilder.tsx | 4 ++ 2 files changed, 46 insertions(+) diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index 6fb5c0e790bf..e33baec94dbe 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -487,6 +487,48 @@ test('should render ParentSize wrapper with height 100% for tabs', async () => { expect(tabPanels.length).toBeGreaterThan(0); }); +test('should apply min-height to the top-level tab drop target so tabs can be dropped on dashboards with content', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + const { container } = render(, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + dashboardState: { ...mockState.dashboardState, editMode: true }, + }), + useDnd: true, + useTheme: true, + }); + + const headerWrapper = container.querySelector( + '[data-test="dashboard-header-wrapper"]', + ); + expect(headerWrapper).toBeInTheDocument(); + + // Verify the StyledHeader CSS includes min-height for the empty drop target. + // Without this, the drop target has zero height and users cannot drag tabs + // onto dashboards that already have content (the Droppable renders an empty + // div and needs explicit min-height to be a valid react-dnd hover target). + const allRules = Array.from(document.styleSheets).flatMap(sheet => { + try { + return Array.from(sheet.cssRules).map(rule => rule.cssText); + } catch { + return []; + } + }); + const emptyDroptargetRule = allRules.find( + rule => + rule.includes('.empty-droptarget') && rule.includes('min-height'), + ); + expect(emptyDroptargetRule).toBeDefined(); +}); + test('should maintain layout when switching between tabs', async () => { (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ 100, diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx index 68dd1971df16..550eb93751df 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx @@ -99,6 +99,10 @@ const StyledHeader = styled.div<{ filterBarWidth: number }>` z-index: 99; max-width: calc(100vw - ${filterBarWidth}px); + .empty-droptarget { + min-height: ${theme.sizeUnit * 4}px; + } + .empty-droptarget:before { position: absolute; content: ''; From 0b442ef9aeda7c70643674ef59c34f043f2381c7 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 30 Apr 2026 13:35:04 -0700 Subject: [PATCH 2/5] fix(dashboard): address review feedback and fix test type errors - Assert the actual .empty-droptarget element exists on the Droppable (addresses Copilot review feedback on PR #39423) - Add @emotion/jest type reference to fix pre-existing toHaveStyleRule TS errors in this test file - Prettier formatting fix Co-Authored-By: Claude Opus 4.6 (1M context) --- .../DashboardBuilder.test.tsx | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index e33baec94dbe..a5ffa196a537 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +/// import fetchMock from 'fetch-mock'; import { fireEvent, @@ -511,10 +512,17 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr ); expect(headerWrapper).toBeInTheDocument(); - // Verify the StyledHeader CSS includes min-height for the empty drop target. - // Without this, the drop target has zero height and users cannot drag tabs - // onto dashboards that already have content (the Droppable renders an empty - // div and needs explicit min-height to be a valid react-dnd hover target). + // The Droppable inside the header should have the empty-droptarget class + // when there are no top-level tabs and edit mode is active. Without this + // class (and its associated min-height CSS rule), the drop target has zero + // height and users cannot drag tabs onto dashboards that already have + // content. + const droptarget = headerWrapper!.querySelector('.empty-droptarget'); + expect(droptarget).toBeInTheDocument(); + + // Verify the StyledHeader CSS defines min-height for .empty-droptarget. + // getComputedStyle doesn't work in jsdom for styled-components injected + // styles, so we check the CSSOM directly. const allRules = Array.from(document.styleSheets).flatMap(sheet => { try { return Array.from(sheet.cssRules).map(rule => rule.cssText); @@ -523,8 +531,7 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr } }); const emptyDroptargetRule = allRules.find( - rule => - rule.includes('.empty-droptarget') && rule.includes('min-height'), + rule => rule.includes('.empty-droptarget') && rule.includes('min-height'), ); expect(emptyDroptargetRule).toBeDefined(); }); From 3fe59024efb30013c4f1b0399ac78af83e38adb6 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Sat, 2 May 2026 17:14:47 -0700 Subject: [PATCH 3/5] fix(dashboard): use toHaveStyleRule and getByTestId in droptarget test Replace CSSOM scan with @emotion/jest toHaveStyleRule matcher using the target option to verify the .empty-droptarget rule on StyledHeader. Switch from container.querySelector to getByTestId for consistency with other tests in this file. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../DashboardBuilder.test.tsx | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index a5ffa196a537..f05e03671ddf 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -496,7 +496,7 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); - const { container } = render(, { + const { getByTestId } = render(, { useRedux: true, store: storeWithState({ ...mockState, @@ -507,33 +507,22 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr useTheme: true, }); - const headerWrapper = container.querySelector( - '[data-test="dashboard-header-wrapper"]', - ); - expect(headerWrapper).toBeInTheDocument(); + const headerWrapper = getByTestId('dashboard-header-wrapper'); // The Droppable inside the header should have the empty-droptarget class // when there are no top-level tabs and edit mode is active. Without this // class (and its associated min-height CSS rule), the drop target has zero // height and users cannot drag tabs onto dashboards that already have // content. - const droptarget = headerWrapper!.querySelector('.empty-droptarget'); + const droptarget = headerWrapper.querySelector('.empty-droptarget'); expect(droptarget).toBeInTheDocument(); // Verify the StyledHeader CSS defines min-height for .empty-droptarget. - // getComputedStyle doesn't work in jsdom for styled-components injected - // styles, so we check the CSSOM directly. - const allRules = Array.from(document.styleSheets).flatMap(sheet => { - try { - return Array.from(sheet.cssRules).map(rule => rule.cssText); - } catch { - return []; - } - }); - const emptyDroptargetRule = allRules.find( - rule => rule.includes('.empty-droptarget') && rule.includes('min-height'), + expect(headerWrapper).toHaveStyleRule( + 'min-height', + expect.stringMatching(/\S+/), + { target: '.empty-droptarget' }, ); - expect(emptyDroptargetRule).toBeDefined(); }); test('should maintain layout when switching between tabs', async () => { From 93066ade5266be4ecd5a1b0e351af42984f06427 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 4 May 2026 12:26:09 -0700 Subject: [PATCH 4/5] fix(dashboard): assert concrete min-height value and centralize emotion-jest types - Assert min-height is '16px' (theme.sizeUnit * 4) instead of /\S+/ to catch zero-height regressions - Move @emotion/jest type reference from per-file directive to src/types/emotion-jest.d.ts for global availability Co-Authored-By: Claude Opus 4.6 (1M context) --- .../DashboardBuilder.test.tsx | 12 +++++------ superset-frontend/src/types/emotion-jest.d.ts | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 superset-frontend/src/types/emotion-jest.d.ts diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index f05e03671ddf..9fbc78658ae9 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -/// import fetchMock from 'fetch-mock'; import { fireEvent, @@ -517,12 +516,11 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr const droptarget = headerWrapper.querySelector('.empty-droptarget'); expect(droptarget).toBeInTheDocument(); - // Verify the StyledHeader CSS defines min-height for .empty-droptarget. - expect(headerWrapper).toHaveStyleRule( - 'min-height', - expect.stringMatching(/\S+/), - { target: '.empty-droptarget' }, - ); + // Verify the StyledHeader CSS defines a non-zero min-height for + // .empty-droptarget (theme.sizeUnit * 4 = 16px with the default theme). + expect(headerWrapper).toHaveStyleRule('min-height', '16px', { + target: '.empty-droptarget', + }); }); test('should maintain layout when switching between tabs', async () => { diff --git a/superset-frontend/src/types/emotion-jest.d.ts b/superset-frontend/src/types/emotion-jest.d.ts new file mode 100644 index 000000000000..c6d15cfb2ddd --- /dev/null +++ b/superset-frontend/src/types/emotion-jest.d.ts @@ -0,0 +1,20 @@ +/** + * 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 b71c556560304a3c6cb002982d9a948726af498a Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 5 May 2026 16:39:32 -0700 Subject: [PATCH 5/5] fix(dashboard): derive min-height assertion from theme.sizeUnit Use supersetTheme.sizeUnit * 4 instead of hard-coded '16px' so the test stays in sync with the source rule and resilient to theme token changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../DashboardBuilder/DashboardBuilder.test.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx index 9fbc78658ae9..e56d006efee3 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx @@ -24,6 +24,7 @@ import { screen, } from 'spec/helpers/testing-library'; import { FeatureFlag } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; import { OPEN_FILTER_BAR_WIDTH, CLOSED_FILTER_BAR_WIDTH, @@ -517,10 +518,15 @@ test('should apply min-height to the top-level tab drop target so tabs can be dr expect(droptarget).toBeInTheDocument(); // Verify the StyledHeader CSS defines a non-zero min-height for - // .empty-droptarget (theme.sizeUnit * 4 = 16px with the default theme). - expect(headerWrapper).toHaveStyleRule('min-height', '16px', { - target: '.empty-droptarget', - }); + // .empty-droptarget, derived from theme.sizeUnit * 4 to stay in sync + // with the source rule in DashboardBuilder.tsx. + expect(headerWrapper).toHaveStyleRule( + 'min-height', + `${supersetTheme.sizeUnit * 4}px`, + { + target: '.empty-droptarget', + }, + ); }); test('should maintain layout when switching between tabs', async () => {