This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 810d6ff4807 fix(sqllab): Resolve stale closure bug causing text
selection to break (#37550)
810d6ff4807 is described below
commit 810d6ff4807f6e2f236a79b7c92aa1a9f2a50d97
Author: Michael S. Molina <[email protected]>
AuthorDate: Thu Jan 29 13:09:26 2026 -0300
fix(sqllab): Resolve stale closure bug causing text selection to break
(#37550)
---
.../EditorWrapper/EditorWrapper.test.tsx | 55 +++++-
.../src/SqlLab/components/EditorWrapper/index.tsx | 33 +++-
.../src/core/editors/AceEditorProvider.test.tsx | 191 +++++++++++++++++++++
.../src/core/editors/AceEditorProvider.tsx | 79 ++++++---
4 files changed, 327 insertions(+), 31 deletions(-)
diff --git
a/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx
b/superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx
index 3252f716579..e903d97f262 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 e6b5ac1afea..0d7002de081 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/core/editors/AceEditorProvider.test.tsx
b/superset-frontend/src/core/editors/AceEditorProvider.test.tsx
new file mode 100644
index 00000000000..7f3b422639d
--- /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<string, (() => 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 <div data-test="sql-editor" />;
+ }),
+ JsonEditor: () => <div data-test="json-editor" />,
+ MarkdownEditor: () => <div data-test="markdown-editor" />,
+ CssEditor: () => <div data-test="css-editor" />,
+ ConfigEditor: () => <div data-test="config-editor" />,
+}));
+
+import AceEditorProvider from './AceEditorProvider';
+
+const render = (ui: ReactElement) =>
+ rtlRender(<ThemeProvider theme={supersetTheme}>{ui}</ThemeProvider>);
+
+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<EditorProps> = {}) => {
+ const result = render(<AceEditorProvider {...defaultProps} {...props} />);
+ 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 (
+ <>
+ <button type="button" onClick={() => setUseSecond(true)}>
+ Switch
+ </button>
+ <AceEditorProvider
+ {...defaultProps}
+ onSelectionChange={useSecond ? secondCallback : firstCallback}
+ />
+ </>
+ );
+ };
+
+ render(<CallbackSwitcher />);
+
+ 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 (
+ <>
+ <button type="button" onClick={() => setUseSecond(true)}>
+ Switch
+ </button>
+ <AceEditorProvider
+ {...defaultProps}
+ onCursorPositionChange={useSecond ? secondCallback : firstCallback}
+ />
+ </>
+ );
+ };
+
+ render(<CallbackSwitcher />);
+
+ 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 018be92f2df..6e18e3e0f36 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<EditorHandle,
EditorProps>(
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<EditorHandle,
EditorProps>(
});
}
- // 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
