diff --git a/README.md b/README.md index 617852c19085..e6dd708623d4 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,7 @@ Superset provides: **Craft Beautiful, Dynamic Dashboards** -
+
**No-Code Chart Builder** diff --git a/docs/static/img/screenshots/dashboard.jpg b/docs/static/img/screenshots/dashboard.jpg new file mode 100644 index 000000000000..9062d7a479d2 Binary files /dev/null and b/docs/static/img/screenshots/dashboard.jpg differ diff --git a/docs/static/img/screenshots/explore.jpg b/docs/static/img/screenshots/explore.jpg index cf2b242c35bc..8ac41b65796a 100644 Binary files a/docs/static/img/screenshots/explore.jpg and b/docs/static/img/screenshots/explore.jpg differ diff --git a/docs/static/img/screenshots/gallery.jpg b/docs/static/img/screenshots/gallery.jpg index 345302de689e..2312bd37982d 100644 Binary files a/docs/static/img/screenshots/gallery.jpg and b/docs/static/img/screenshots/gallery.jpg differ diff --git a/docs/static/img/screenshots/sql_lab.jpg b/docs/static/img/screenshots/sql_lab.jpg index b66bcc52b7fa..d8f5a964adba 100644 Binary files a/docs/static/img/screenshots/sql_lab.jpg and b/docs/static/img/screenshots/sql_lab.jpg differ diff --git a/superset-frontend/package.json b/superset-frontend/package.json index c7e2a5f04a35..a20f29f344ae 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -74,6 +74,7 @@ "playwright:headed": "playwright test --headed", "playwright:debug": "playwright test --debug", "playwright:report": "playwright show-report", + "docs:screenshots": "playwright test --config=playwright/generators/playwright.config.ts docs/", "prettier": "npm run _prettier -- --write", "prettier-check": "npm run _prettier -- --check", "prod": "npm run build", diff --git a/superset-frontend/playwright/generators/docs/docs-screenshots.spec.ts b/superset-frontend/playwright/generators/docs/docs-screenshots.spec.ts new file mode 100644 index 000000000000..5e3814b849e4 --- /dev/null +++ b/superset-frontend/playwright/generators/docs/docs-screenshots.spec.ts @@ -0,0 +1,230 @@ +/** + * 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. + */ + +/** + * Documentation Screenshot Generator + * + * Captures screenshots for the Superset documentation site and README. + * Depends on example data loaded via `superset load_examples`. + * + * Run locally: + * cd superset-frontend + * npm run docs:screenshots + * + * Or directly: + * npx playwright test --config=playwright/generators/playwright.config.ts docs/ + * + * Screenshots are saved to docs/static/img/screenshots/. + */ + +import path from 'path'; +import { test, expect } from '@playwright/test'; +import { URL } from '../../utils/urls'; + +const SCREENSHOTS_DIR = path.resolve( + __dirname, + '../../../../docs/static/img/screenshots', +); + +test('chart gallery screenshot', async ({ page }) => { + await page.goto(URL.CHART_ADD); + + // Wait for chart creation page to load + await expect(page.getByText('Choose chart type')).toBeVisible({ + timeout: 15000, + }); + await page.getByRole('tab', { name: 'All charts' }).click(); + + // Wait for viz gallery to render chart type thumbnails + const vizGallery = page.locator('.viz-gallery'); + await expect(vizGallery).toBeVisible(); + await expect( + vizGallery.locator('[data-test="viztype-selector-container"]').first(), + ).toBeVisible(); + + await vizGallery.screenshot({ + path: path.join(SCREENSHOTS_DIR, 'gallery.jpg'), + type: 'jpeg', + }); +}); + +test('dashboard screenshot', async ({ page }) => { + // Navigate to Sales Dashboard via the dashboard list (slug is null) + await page.goto(URL.DASHBOARD_LIST); + const searchInput = page.getByPlaceholder('Type a value'); + await expect(searchInput).toBeVisible({ timeout: 15000 }); + await searchInput.fill('Sales Dashboard'); + await searchInput.press('Enter'); + + // Click the Sales Dashboard link + const dashboardLink = page.getByRole('link', { name: /sales dashboard/i }); + await expect(dashboardLink).toBeVisible({ timeout: 10000 }); + await dashboardLink.click(); + + // Wait for dashboard to fully render + const dashboardWrapper = page.locator( + '[data-test="dashboard-content-wrapper"]', + ); + await expect(dashboardWrapper).toBeVisible({ timeout: 30000 }); + + // Wait for chart holders to appear, then wait for all loading spinners to clear + await expect( + page.locator('.dashboard-component-chart-holder').first(), + ).toBeVisible({ timeout: 15000 }); + await expect( + dashboardWrapper.locator('[data-test="loading-indicator"]'), + ).toHaveCount(0, { timeout: 30000 }); + + // Wait for at least one chart to finish rendering (ECharts renders to canvas) + await expect( + page.locator('.dashboard-component-chart-holder canvas').first(), + ).toBeVisible({ timeout: 15000 }); + + // Open the filter bar (collapsed by default) + const expandButton = page.locator('[data-test="filter-bar__expand-button"]'); + if (await expandButton.isVisible()) { + await expandButton.click(); + // Wait for filter bar content to expand and render filter controls + await expect( + page.locator('[data-test="filter-bar__collapsable"]'), + ).toBeVisible({ timeout: 5000 }); + await expect( + page.locator('[data-test="filterbar-action-buttons"]'), + ).toBeVisible({ timeout: 5000 }); + } + + await page.screenshot({ + path: path.join(SCREENSHOTS_DIR, 'dashboard.jpg'), + type: 'jpeg', + fullPage: true, + }); +}); + +test('chart editor screenshot', async ({ page }) => { + await page.goto(URL.CHART_LIST); + + // Search for the Scatter Plot chart by name + const searchInput = page.getByPlaceholder('Type a value'); + await expect(searchInput).toBeVisible({ timeout: 15000 }); + await searchInput.fill('Scatter Plot'); + await searchInput.press('Enter'); + + // Click the Scatter Plot link to open explore + const chartLink = page.getByRole('link', { name: /scatter plot/i }); + await expect(chartLink).toBeVisible({ timeout: 10000 }); + await chartLink.click(); + + // Wait for explore page to fully load + await page.waitForURL('**/explore/**', { timeout: 15000 }); + const sliceContainer = page.locator('[data-test="slice-container"]'); + await expect(sliceContainer).toBeVisible({ timeout: 15000 }); + + // Wait for the chart to finish rendering (loading spinners clear, chart content appears) + await expect( + sliceContainer.locator('[data-test="loading-indicator"]'), + ).toHaveCount(0, { timeout: 15000 }); + await expect(sliceContainer.locator('canvas, svg').first()).toBeVisible({ + timeout: 15000, + }); + + await page.screenshot({ + path: path.join(SCREENSHOTS_DIR, 'explore.jpg'), + type: 'jpeg', + fullPage: true, + }); +}); + +test('SQL Lab screenshot', async ({ page }) => { + // SQL Lab has many interactive steps (schema, table, query, results) — allow extra time + test.setTimeout(90000); + await page.goto(URL.SQLLAB); + + // SQL Lab may open with no active query tab — create one if needed + const addTabButton = page.getByRole('tab', { name: /add a new tab/i }); + const aceEditor = page.locator('.ace_content'); + + // Wait for either the editor or the "add tab" prompt + await expect(addTabButton.or(aceEditor)).toBeVisible({ timeout: 15000 }); + + // If no editor is visible, click "Add a new tab" to create a query tab + if (await addTabButton.isVisible()) { + await addTabButton.click(); + } + await expect(aceEditor).toBeVisible({ timeout: 15000 }); + + // Select the "public" schema so we can pick a table from the left panel + const schemaSelect = page.locator('#select-schema'); + await expect(schemaSelect).toBeEnabled({ timeout: 10000 }); + await schemaSelect.click({ force: true }); + await schemaSelect.fill('public'); + await page.getByRole('option', { name: 'public' }).click(); + + // Wait for table list to load after schema change, then select birth_names + const tableSelectWrapper = page + .locator('.ant-select') + .filter({ has: page.locator('#select-table') }); + await expect(tableSelectWrapper).toBeVisible({ timeout: 10000 }); + await tableSelectWrapper.click(); + await page.keyboard.type('birth_names'); + // Wait for the filtered option to appear in the DOM, then select it + const tableOption = page + .locator('.ant-select-dropdown [role="option"]') + .filter({ hasText: 'birth_names' }); + await expect(tableOption).toBeAttached({ timeout: 10000 }); + await page.keyboard.press('Enter'); + + // Wait for table schema to load and show columns in the left panel + await expect(page.locator('[data-test="col-name"]').first()).toBeVisible({ + timeout: 15000, + }); + + // Close the table dropdown by clicking elsewhere, then switch to the query tab + await page.locator('[data-test="sql-editor-tabs"]').first().click(); + await page.getByText('Untitled Query').first().click(); + + // Write a multi-line SELECT with explicit columns to fill the editor + await aceEditor.click(); + const editor = page.getByRole('textbox', { name: /cursor/i }); + await editor.fill( + 'SELECT\n ds,\n name,\n gender,\n state,\n num\nFROM birth_names\nLIMIT 100', + ); + + // Run the query + const runButton = page.getByText('Run', { exact: true }); + await expect(runButton).toBeVisible(); + await runButton.click(); + + // Wait for results to appear (look for the "N rows" badge in the results panel) + await expect(page.getByText(/\d+ rows/)).toBeVisible({ + timeout: 30000, + }); + + // Switch to the Results tab to show the query output + await page.getByRole('tab', { name: 'Results' }).click(); + + // Move mouse away from buttons to dismiss any tooltips, then wait for them to disappear + await page.mouse.move(0, 0); + await expect(page.getByRole('tooltip')).toHaveCount(0, { timeout: 2000 }); + + await page.screenshot({ + path: path.join(SCREENSHOTS_DIR, 'sql_lab.jpg'), + type: 'jpeg', + fullPage: true, + }); +}); diff --git a/superset-frontend/playwright/generators/playwright.config.ts b/superset-frontend/playwright/generators/playwright.config.ts new file mode 100644 index 000000000000..54b8b4be7028 --- /dev/null +++ b/superset-frontend/playwright/generators/playwright.config.ts @@ -0,0 +1,81 @@ +/** + * 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. + */ + +/** + * Playwright config for documentation generators (screenshots, etc.) + * + * Separate from the main test config so generators are never picked up + * by CI test sweeps. Run via: + * npm run docs:screenshots + */ + +/// + +import path from 'path'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { defineConfig } from '@playwright/test'; + +const serverURL = process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088'; +const baseURL = serverURL.endsWith('/') ? serverURL : `${serverURL}/`; + +export default defineConfig({ + testDir: '.', + + globalSetup: '../global-setup.ts', + + timeout: 90000, + expect: { timeout: 30000 }, + + fullyParallel: false, + workers: 1, + retries: 0, + + reporter: [['list']], + + use: { + baseURL, + + headless: true, + viewport: { width: 1280, height: 1024 }, + + screenshot: 'off', + video: 'off', + trace: 'off', + }, + + projects: [ + { + name: 'docs-generators', + use: { + browserName: 'chromium', + testIdAttribute: 'data-test', + storageState: path.resolve(__dirname, '../.auth/user.json'), + }, + }, + ], + + webServer: process.env.CI + ? undefined + : { + command: `curl -f ${serverURL}/health`, + url: `${serverURL}/health`, + reuseExistingServer: true, + timeout: 5000, + }, +}); diff --git a/superset-frontend/playwright/utils/urls.ts b/superset-frontend/playwright/utils/urls.ts index d83e33f755fb..fa5be71c8f17 100644 --- a/superset-frontend/playwright/utils/urls.ts +++ b/superset-frontend/playwright/utils/urls.ts @@ -28,8 +28,11 @@ * = 'http://localhost:8088/app/prefix/tablemodelview/list' */ export const URL = { - DATASET_LIST: 'tablemodelview/list', + CHART_ADD: 'chart/add', + CHART_LIST: 'chart/list/', DASHBOARD_LIST: 'dashboard/list/', + DATASET_LIST: 'tablemodelview/list', LOGIN: 'login/', + SQLLAB: 'sqllab', WELCOME: 'superset/welcome/', } as const; diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx index eac5d13df7cf..91104ba7502e 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx @@ -485,6 +485,89 @@ describe('DeckMulti Component Rendering', () => { }); }); + it('should include dashboardId in child slice requests when present', async () => { + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + dashboardId: 123, // Simulate embedded dashboard context + }, + }; + + renderWithProviders(); + + // Wait for child slice requests + await waitFor(() => { + expect(SupersetClient.get).toHaveBeenCalled(); + }); + + // Check that all requests include the dashboardId + const calls = (SupersetClient.get as jest.Mock).mock.calls; + calls.forEach(call => { + const url = call[0].endpoint; + const urlParams = new URLSearchParams(url.split('?')[1]); + const formDataString = urlParams.get('form_data'); + const formData = JSON.parse(formDataString || '{}'); + expect(formData.dashboardId).toBe(123); + }); + }); + + it('should not include dashboardId when not present', async () => { + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + // No dashboardId + }, + }; + + renderWithProviders(); + + // Wait for child slice requests + await waitFor(() => { + expect(SupersetClient.get).toHaveBeenCalled(); + }); + + // Check that requests don't include dashboardId + const calls = (SupersetClient.get as jest.Mock).mock.calls; + calls.forEach(call => { + const url = call[0].endpoint; + const formData = JSON.parse( + new URLSearchParams(url.split('?')[1]).get('form_data') || '{}', + ); + expect(formData.dashboardId).toBeUndefined(); + }); + }); + + it('should preserve dashboardId through filter updates', async () => { + const props = { + ...baseMockProps, + formData: { + ...baseMockProps.formData, + dashboardId: 456, + extra_filters: [{ col: 'test', op: 'IN' as const, val: ['value'] }], + }, + }; + + renderWithProviders(); + + // Wait for child slice requests + await waitFor(() => { + expect(SupersetClient.get).toHaveBeenCalled(); + }); + + // Verify dashboardId is preserved with filters + const calls = (SupersetClient.get as jest.Mock).mock.calls; + calls.forEach(call => { + const url = call[0].endpoint; + const formData = JSON.parse( + new URLSearchParams(url.split('?')[1]).get('form_data') || '{}', + ); + expect(formData.dashboardId).toBe(456); + expect(formData.extra_filters).toBeDefined(); + }); + }); + it('should handle viewport changes', async () => { const { rerender } = renderWithProviders(); diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx index 1ee7ee12707b..0fdc53f2b975 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx @@ -287,6 +287,8 @@ const DeckMulti = (props: DeckMultiProps) => { ...subslice.form_data, extra_filters: extraFilters, adhoc_filters: adhocFilters, + // Preserve dashboard context for embedded mode permissions + ...(formData.dashboardId && { dashboardId: formData.dashboardId }), }, } as any as JsonObject & { slice_id: number }; diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.test.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.test.tsx new file mode 100644 index 000000000000..96f7ca4e7604 --- /dev/null +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.test.tsx @@ -0,0 +1,120 @@ +/** + * 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. + */ +// eslint-disable-next-line import/no-extraneous-dependencies +import '@testing-library/jest-dom'; + +jest.mock('../../DeckGLContainer', () => ({ + DeckGLContainerStyledWrapper: ({ children }: any) => ( +
{children}
+ ), +})); + +jest.mock('../../factory', () => ({ + createDeckGLComponent: jest.fn(() => () => null), + GetLayerType: {}, +})); + +import { getLayer, getPoints, getHighlightLayer } from './Path'; + +const mockFormData = { + datasource: 'test_datasource', + viz_type: 'deck_path', + color_picker: { r: 0, g: 122, b: 135, a: 1 }, + line_width: 150, + line_width_unit: 'meters', + slice_id: 1, +}; + +const mockPayload = { + data: { + features: [ + { + path: [ + [-122.4, 37.8], + [-122.5, 37.9], + ], + }, + ], + }, +}; + +test('getLayer uses line_width_unit from formData', () => { + const layer = getLayer({ + formData: mockFormData, + payload: mockPayload, + onContextMenu: jest.fn(), + filterState: undefined, + setDataMask: jest.fn(), + setTooltip: jest.fn(), + emitCrossFilters: false, + }); + + expect(layer.props.widthUnits).toBe('meters'); +}); + +test('getLayer uses pixels when line_width_unit is pixels', () => { + const layer = getLayer({ + formData: { ...mockFormData, line_width_unit: 'pixels' }, + payload: mockPayload, + onContextMenu: jest.fn(), + filterState: undefined, + setDataMask: jest.fn(), + setTooltip: jest.fn(), + emitCrossFilters: false, + }); + + expect(layer.props.widthUnits).toBe('pixels'); +}); + +test('getHighlightLayer uses line_width_unit from formData', () => { + const layer = getHighlightLayer({ + formData: mockFormData, + payload: mockPayload, + filterState: { value: [] }, + onContextMenu: jest.fn(), + setDataMask: jest.fn(), + setTooltip: jest.fn(), + emitCrossFilters: false, + }); + + expect(layer.props.widthUnits).toBe('meters'); +}); + +test('getPoints extracts points from path data', () => { + const data = [ + { + path: [ + [0, 0], + [1, 1], + ], + }, + { + path: [ + [2, 2], + [3, 3], + ], + }, + ]; + + const points = getPoints(data); + + expect(points).toHaveLength(4); + expect(points[0]).toEqual([0, 0]); + expect(points[2]).toEqual([2, 2]); +}); diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts index 916ad8d7daa4..194b9d928020 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts @@ -75,7 +75,7 @@ const config: ControlPanelConfig = { config: { type: 'SelectControl', label: t('Line width unit'), - default: 'pixels', + default: 'meters', choices: [ ['meters', t('meters')], ['pixels', t('pixels')], diff --git a/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx b/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx index 3252f716579d..e903d97f2622 100644 --- a/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx +++ b/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx @@ -17,7 +17,12 @@ * under the License. */ import reducerIndex from 'spec/helpers/reducerIndex'; -import { render, waitFor, createStore } from 'spec/helpers/testing-library'; +import { + render, + waitFor, + createStore, + act, +} from 'spec/helpers/testing-library'; import { QueryEditor } from 'src/SqlLab/types'; import { Store } from 'redux'; import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; @@ -26,6 +31,7 @@ import type { editors } from '@apache-superset/core'; import { queryEditorSetCursorPosition, queryEditorSetDb, + queryEditorSetSelectedText, } from 'src/SqlLab/actions/sqlLab'; import fetchMock from 'fetch-mock'; @@ -124,4 +130,51 @@ describe('EditorWrapper', () => { store.dispatch(queryEditorSetDb(defaultQueryEditor, 2)); expect(MockEditorHost).toHaveBeenCalledTimes(renderCount + 1); }); + + test('clears selectedText when selection becomes empty', async () => { + const store = createStore(initialState, reducerIndex); + // Set initial selected text in store + store.dispatch( + queryEditorSetSelectedText(defaultQueryEditor, 'SELECT * FROM table'), + ); + setup(defaultQueryEditor, store); + + await waitFor(() => expect(MockEditorHost).toHaveBeenCalled()); + + // Get the onSelectionChange and onReady callbacks from the mock + const lastCall = + MockEditorHost.mock.calls[MockEditorHost.mock.calls.length - 1][0]; + const { onSelectionChange, onReady } = lastCall; + + // Simulate editor ready with a mock handle that returns empty selection + const mockHandle = { + getSelectedText: jest.fn().mockReturnValue(''), + getValue: jest.fn().mockReturnValue(''), + setValue: jest.fn(), + focus: jest.fn(), + moveCursorToPosition: jest.fn(), + scrollToLine: jest.fn(), + }; + act(() => { + onReady(mockHandle); + }); + + // Simulate selection change with empty selection (cursor moved without selecting) + act(() => { + onSelectionChange([ + { start: { line: 0, column: 5 }, end: { line: 0, column: 5 } }, + ]); + }); + + // Verify selectedText was cleared in the store + await waitFor(() => { + const state = store.getState() as unknown as { + sqlLab: { queryEditors: QueryEditor[] }; + }; + const editor = state.sqlLab.queryEditors.find( + qe => qe.id === defaultQueryEditor.id, + ); + expect(editor?.selectedText).toBeFalsy(); + }); + }); }); diff --git a/superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx b/superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx index e6b5ac1afea7..0d7002de0818 100644 --- a/superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx +++ b/superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx @@ -216,16 +216,35 @@ const EditorWrapper = ({ end: { line: number; column: number }; }>, ) => { - if (editorHandleRef.current && selections.length > 0) { - const selectedText = editorHandleRef.current.getSelectedText(); - if ( - selectedText !== currentSelectionCache.current && - selectedText.length !== 1 - ) { - dispatch(queryEditorSetSelectedText(queryEditor, selectedText)); + if (!editorHandleRef.current || selections.length === 0) { + return; + } + + const selectedText = editorHandleRef.current.getSelectedText(); + + // Always clear selection state when text is empty, regardless of cache + if (!selectedText) { + if (currentSelectionCache.current) { + dispatch(queryEditorSetSelectedText(queryEditor, null)); } + currentSelectionCache.current = ''; + return; + } + + // Skip 1-character selections to avoid noise from backspace operations + // which briefly select single characters. Trade-off: genuine single-char + // selections won't update Redux, but this is acceptable since running + // a single character as a query is not a practical use case. + if (selectedText.length === 1) { currentSelectionCache.current = selectedText; + return; + } + + // Only dispatch if selection actually changed + if (selectedText !== currentSelectionCache.current) { + dispatch(queryEditorSetSelectedText(queryEditor, selectedText)); } + currentSelectionCache.current = selectedText; }, [dispatch, queryEditor], ); diff --git a/superset-frontend/src/components/ListView/ActionsBar.tsx b/superset-frontend/src/components/ListView/ActionsBar.tsx index 8d798eb81fc1..503b3bbee911 100644 --- a/superset-frontend/src/components/ListView/ActionsBar.tsx +++ b/superset-frontend/src/components/ListView/ActionsBar.tsx @@ -69,7 +69,8 @@ export function ActionsBar({ actions }: ActionsBarProps) { return ( } + icon={} + tooltip={tooltip} {...rest} /> ); diff --git a/superset-frontend/src/core/editors/AceEditorProvider.test.tsx b/superset-frontend/src/core/editors/AceEditorProvider.test.tsx new file mode 100644 index 000000000000..7f3b422639de --- /dev/null +++ b/superset-frontend/src/core/editors/AceEditorProvider.test.tsx @@ -0,0 +1,191 @@ +/** + * 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 { useState, ReactElement } from 'react'; +import { + render as rtlRender, + screen, + waitFor, + cleanup, +} from '@testing-library/react'; +import '@testing-library/jest-dom'; +import userEvent from '@testing-library/user-event'; +import { ThemeProvider, supersetTheme } from '@apache-superset/core/ui'; +import type { editors } from '@apache-superset/core'; + +type EditorProps = editors.EditorProps; + +const mockEventHandlers: Record void) | undefined> = {}; + +const mockEditor = { + focus: jest.fn(), + getCursorPosition: jest.fn(() => ({ row: 1, column: 5 })), + getSelection: jest.fn(() => ({ + getRange: () => ({ + start: { row: 0, column: 0 }, + end: { row: 0, column: 10 }, + }), + })), + commands: { addCommand: jest.fn() }, + selection: { + on: jest.fn((event: string, handler: () => void) => { + mockEventHandlers[event] = handler; + }), + }, +}; + +let mockOnLoadCallback: ((editor: typeof mockEditor) => void) | undefined; + +jest.mock('@superset-ui/core/components', () => ({ + __esModule: true, + FullSQLEditor: jest.fn((props: { onLoad?: () => void }) => { + mockOnLoadCallback = props.onLoad; + return
; + }), + JsonEditor: () =>
, + MarkdownEditor: () =>
, + CssEditor: () =>
, + ConfigEditor: () =>
, +})); + +import AceEditorProvider from './AceEditorProvider'; + +const render = (ui: ReactElement) => + rtlRender({ui}); + +afterEach(() => { + cleanup(); + jest.clearAllMocks(); + mockOnLoadCallback = undefined; + Object.keys(mockEventHandlers).forEach(key => delete mockEventHandlers[key]); +}); + +const defaultProps: EditorProps = { + id: 'test-editor', + value: 'SELECT * FROM table', + onChange: jest.fn(), + language: 'sql', +}; + +const renderEditor = (props: Partial = {}) => { + const result = render(); + if (mockOnLoadCallback) { + mockOnLoadCallback(mockEditor); + } + return result; +}; + +test('onSelectionChange uses latest callback after prop change', async () => { + const firstCallback = jest.fn(); + const secondCallback = jest.fn(); + + const CallbackSwitcher = () => { + const [useSecond, setUseSecond] = useState(false); + return ( + <> + + + + ); + }; + + render(); + + if (mockOnLoadCallback) { + mockOnLoadCallback(mockEditor); + } + + await waitFor(() => expect(mockEventHandlers.changeSelection).toBeDefined()); + + mockEventHandlers.changeSelection!(); + expect(firstCallback).toHaveBeenCalled(); + expect(secondCallback).not.toHaveBeenCalled(); + firstCallback.mockClear(); + + await userEvent.click(screen.getByRole('button', { name: /switch/i })); + mockEventHandlers.changeSelection!(); + + expect(secondCallback).toHaveBeenCalled(); + expect(firstCallback).not.toHaveBeenCalled(); +}); + +test('onCursorPositionChange uses latest callback after prop change', async () => { + const firstCallback = jest.fn(); + const secondCallback = jest.fn(); + + const CallbackSwitcher = () => { + const [useSecond, setUseSecond] = useState(false); + return ( + <> + + + + ); + }; + + render(); + + if (mockOnLoadCallback) { + mockOnLoadCallback(mockEditor); + } + + await waitFor(() => expect(mockEventHandlers.changeCursor).toBeDefined()); + + mockEventHandlers.changeCursor!(); + expect(firstCallback).toHaveBeenCalled(); + expect(secondCallback).not.toHaveBeenCalled(); + firstCallback.mockClear(); + + await userEvent.click(screen.getByRole('button', { name: /switch/i })); + mockEventHandlers.changeCursor!(); + + expect(secondCallback).toHaveBeenCalled(); + expect(firstCallback).not.toHaveBeenCalled(); +}); + +test('cursor position callback receives correct position format', async () => { + const onCursorPositionChange = jest.fn(); + renderEditor({ onCursorPositionChange }); + + await waitFor(() => expect(mockEventHandlers.changeCursor).toBeDefined()); + mockEventHandlers.changeCursor!(); + + expect(onCursorPositionChange).toHaveBeenCalledWith({ line: 1, column: 5 }); +}); + +test('selection callback receives correct range format', async () => { + const onSelectionChange = jest.fn(); + renderEditor({ onSelectionChange }); + + await waitFor(() => expect(mockEventHandlers.changeSelection).toBeDefined()); + mockEventHandlers.changeSelection!(); + + expect(onSelectionChange).toHaveBeenCalledWith([ + { start: { line: 0, column: 0 }, end: { line: 0, column: 10 } }, + ]); +}); diff --git a/superset-frontend/src/core/editors/AceEditorProvider.tsx b/superset-frontend/src/core/editors/AceEditorProvider.tsx index 018be92f2df9..6e18e3e0f369 100644 --- a/superset-frontend/src/core/editors/AceEditorProvider.tsx +++ b/superset-frontend/src/core/editors/AceEditorProvider.tsx @@ -31,6 +31,7 @@ import { useCallback, useImperativeHandle, forwardRef, + useMemo, type Ref, } from 'react'; import type AceEditor from 'react-ace'; @@ -222,15 +223,42 @@ const AceEditorProvider = forwardRef( new Map(), ); - // Create the handle - const handle = createAceEditorHandle(aceEditorRef, completionProviders); + // Use refs to store latest callbacks to avoid stale closures in event listeners + const onCursorPositionChangeRef = useRef(onCursorPositionChange); + const onSelectionChangeRef = useRef(onSelectionChange); + + // Keep refs up to date + useEffect(() => { + onCursorPositionChangeRef.current = onCursorPositionChange; + }, [onCursorPositionChange]); + + useEffect(() => { + onSelectionChangeRef.current = onSelectionChange; + }, [onSelectionChange]); + + // Create the handle (memoized to prevent recreation on every render) + const handle = useMemo( + () => createAceEditorHandle(aceEditorRef, completionProviders), + [], + ); // Expose handle via ref - useImperativeHandle(ref, () => handle, []); + useImperativeHandle(ref, () => handle, [handle]); - // Notify when ready + // Track if onReady has been called to prevent multiple calls + const onReadyCalledRef = useRef(false); + + // Track if event listeners have been registered to prevent duplicates + const listenersRegisteredRef = useRef(false); + + // Notify when ready (only once) useEffect(() => { - if (onReady && aceEditorRef.current?.editor) { + if ( + onReady && + aceEditorRef.current?.editor && + !onReadyCalledRef.current + ) { + onReadyCalledRef.current = true; onReady(handle); } }, [onReady, handle]); @@ -249,34 +277,39 @@ const AceEditorProvider = forwardRef( }); } - // Set up cursor position change listener - if (onCursorPositionChange) { + // Only register event listeners once to prevent duplicates + if (!listenersRegisteredRef.current) { + listenersRegisteredRef.current = true; + + // Set up cursor position change listener using ref to avoid stale closures editor.selection.on('changeCursor', () => { - const cursor = editor.getCursorPosition(); - onCursorPositionChange({ - line: cursor.row, - column: cursor.column, - }); + if (onCursorPositionChangeRef.current) { + const cursor = editor.getCursorPosition(); + onCursorPositionChangeRef.current({ + line: cursor.row, + column: cursor.column, + }); + } }); - } - // Set up selection change listener - if (onSelectionChange) { + // Set up selection change listener using ref to avoid stale closures editor.selection.on('changeSelection', () => { - const range = editor.getSelection().getRange(); - onSelectionChange([ - { - start: { line: range.start.row, column: range.start.column }, - end: { line: range.end.row, column: range.end.column }, - }, - ]); + if (onSelectionChangeRef.current) { + const range = editor.getSelection().getRange(); + onSelectionChangeRef.current([ + { + start: { line: range.start.row, column: range.start.column }, + end: { line: range.end.row, column: range.end.column }, + }, + ]); + } }); } // Focus the editor editor.focus(); }, - [hotkeys, onCursorPositionChange, onSelectionChange, handle], + [hotkeys, handle], ); // Handle blur diff --git a/superset-frontend/src/pages/ThemeList/index.tsx b/superset-frontend/src/pages/ThemeList/index.tsx index 002e925bf69c..3e2db4615de5 100644 --- a/superset-frontend/src/pages/ThemeList/index.tsx +++ b/superset-frontend/src/pages/ThemeList/index.tsx @@ -386,14 +386,14 @@ function ThemesList({ )} {original.is_system_default && ( - + }> {t('Default')} )} {original.is_system_dark && ( - + }> {t('Dark')} @@ -442,9 +442,7 @@ function ThemesList({ canApply ? { label: 'apply-action', - tooltip: t( - 'Set local theme. Will be applied to your session until unset.', - ), + tooltip: t('Set local theme for testing'), placement: 'bottom', icon: 'ThunderboltOutlined', onClick: handleApply, @@ -453,9 +451,7 @@ function ThemesList({ canEdit ? { label: 'edit-action', - tooltip: original.is_system - ? t('View theme') - : t('Edit theme'), + tooltip: original.is_system ? t('View') : t('Edit'), placement: 'bottom', icon: original.is_system ? 'EyeOutlined' : 'EditOutlined', onClick: handleEdit, @@ -464,7 +460,7 @@ function ThemesList({ canExport ? { label: 'export-action', - tooltip: t('Export theme'), + tooltip: t('Export'), placement: 'bottom', icon: 'UploadOutlined', onClick: handleExport, @@ -473,7 +469,7 @@ function ThemesList({ canSetSystemThemes && !original.is_system_default ? { label: 'set-default-action', - tooltip: t('Set as system default theme'), + tooltip: t('Set as default light theme'), placement: 'bottom', icon: 'SunOutlined', onClick: () => handleSetSystemDefault(original), @@ -482,7 +478,7 @@ function ThemesList({ canSetSystemThemes && original.is_system_default ? { label: 'unset-default-action', - tooltip: t('Remove as system default theme'), + tooltip: t('Clear default light theme'), placement: 'bottom', icon: 'StopOutlined', onClick: () => handleUnsetSystemDefault(), @@ -491,7 +487,7 @@ function ThemesList({ canSetSystemThemes && !original.is_system_dark ? { label: 'set-dark-action', - tooltip: t('Set as system dark theme'), + tooltip: t('Set as default dark theme'), placement: 'bottom', icon: 'MoonOutlined', onClick: () => handleSetSystemDark(original), @@ -500,7 +496,7 @@ function ThemesList({ canSetSystemThemes && original.is_system_dark ? { label: 'unset-dark-action', - tooltip: t('Remove as system dark theme'), + tooltip: t('Clear default dark theme'), placement: 'bottom', icon: 'StopOutlined', onClick: () => handleUnsetSystemDark(), diff --git a/superset/examples/deckgl_demo/charts/Deck.gl_Path.yaml b/superset/examples/deckgl_demo/charts/Deck.gl_Path.yaml index 993eb802ef58..c8292bf6e4da 100644 --- a/superset/examples/deckgl_demo/charts/Deck.gl_Path.yaml +++ b/superset/examples/deckgl_demo/charts/Deck.gl_Path.yaml @@ -35,6 +35,7 @@ params: line_column: path_json line_type: json line_width: 150 + line_width_unit: meters mapbox_style: https://tile.openstreetmap.org/{z}/{x}/{y}.png reverse_long_lat: false row_limit: 5000 diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 326254acb2eb..ac11ecd62d0d 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -32,7 +32,7 @@ from typing import Any, Callable, TYPE_CHECKING, TypeVar from flask import g -from flask_appbuilder.security.sqla.models import User +from flask_appbuilder.security.sqla.models import Group, User if TYPE_CHECKING: from superset.connectors.sqla.models import SqlaTable @@ -43,6 +43,50 @@ logger = logging.getLogger(__name__) +def load_user_with_relationships( + username: str | None = None, email: str | None = None +) -> User | None: + """ + Load a user with all relationships needed for permission checks. + + This function eagerly loads User.roles, User.groups, and Group.roles + to prevent detached instance errors when the session is closed/rolled back. + + IMPORTANT: Always use this function instead of security_manager.find_user() + when loading users for MCP tool execution. The find_user() method doesn't + eagerly load Group.roles, causing "detached instance" errors when permission + checks access group.roles after the session is rolled back. + + Args: + username: The username to look up (optional if email provided) + email: The email to look up (optional if username provided) + + Returns: + User object with relationships loaded, or None if not found + + Raises: + ValueError: If neither username nor email is provided + """ + if not username and not email: + raise ValueError("Either username or email must be provided") + + from sqlalchemy.orm import joinedload + + from superset.extensions import db + + query = db.session.query(User).options( + joinedload(User.roles), + joinedload(User.groups).joinedload(Group.roles), + ) + + if username: + query = query.filter(User.username == username) + else: + query = query.filter(User.email == email) + + return query.first() + + def get_user_from_request() -> User: """ Get the current user for the MCP tool request. @@ -58,9 +102,6 @@ def get_user_from_request() -> User: ValueError: If user cannot be authenticated or found """ from flask import current_app - from sqlalchemy.orm import joinedload - - from superset.extensions import db # First check if user is already set by Preset workspace middleware if hasattr(g, "user") and g.user: @@ -76,14 +117,8 @@ def get_user_from_request() -> User: "MCP_DEV_USERNAME for development." ) - # Query user directly with eager loading to ensure fresh session-bound object - # Do NOT use security_manager.find_user() as it may return cached/detached user - user = ( - db.session.query(User) - .options(joinedload(User.roles), joinedload(User.groups)) - .filter(User.username == username) - .first() - ) + # Use helper function to load user with all required relationships + user = load_user_with_relationships(username) if not user: raise ValueError( @@ -166,20 +201,6 @@ def _cleanup_session_on_error() -> None: logger.warning("Error cleaning up session after exception: %s", e) -def _cleanup_session_finally() -> None: - """Clean up database session in finally block.""" - from superset.extensions import db - - # Rollback active session (no exception occurred) - # Do NOT call remove() on success to avoid detaching user - try: - if db.session.is_active: - # pylint: disable=consider-using-transaction - db.session.rollback() - except Exception as e: - logger.warning("Error in finally block: %s", e) - - def mcp_auth_hook(tool_func: F) -> F: # noqa: C901 """ Authentication and authorization decorator for MCP tools. @@ -240,8 +261,6 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: except Exception: _cleanup_session_on_error() raise - finally: - _cleanup_session_finally() wrapper = async_wrapper @@ -272,8 +291,6 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any: except Exception: _cleanup_session_on_error() raise - finally: - _cleanup_session_finally() wrapper = sync_wrapper