codeant-ai-for-open-source[bot] commented on code in PR #37550:
URL: https://github.com/apache/superset/pull/37550#discussion_r2741680367
##########
superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx:
##########
@@ -124,4 +125,47 @@ 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(),
+ };
+ onReady(mockHandle);
+
+ // Simulate selection change with empty selection (cursor moved without
selecting)
+ onSelectionChange([
+ { start: { line: 0, column: 5 }, end: { line: 0, column: 5 } },
+ ]);
Review Comment:
**Suggestion:** Race condition / React update warning: calling
`onReady(mockHandle)` and `onSelectionChange(...)` synchronously can trigger
React state updates outside of the test harness' act wrapper and cause flaky
tests or warnings. Wrap these calls in `waitFor` (already imported) so they run
inside the testing-library/React act wrapper and ensure state updates are
flushed before assertions. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ React act warnings printed during tests
- ⚠️ Flaky test behavior for EditorWrapper.test.tsx
```
</details>
```suggestion
await waitFor(() => {
onReady(mockHandle);
// Simulate selection change with empty selection (cursor moved
without selecting)
onSelectionChange([
{ start: { line: 0, column: 5 }, end: { line: 0, column: 5 } },
]);
return true;
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test file
superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx
with Jest
(the test titled "clears selectedText when selection becomes empty").
2. The test waits for the mock to be mounted at ~line 137, then obtains
callbacks at
~lines 140-142 (onSelectionChange and onReady).
3. The test calls onReady(mockHandle) and then onSelectionChange(...)
synchronously at
lines 153-158. These calls may trigger React state updates inside
EditorWrapper that occur
outside the testing-library/React act wrapper.
4. When running the test suite, this pattern can produce React "act()"
warnings or
occasional flakiness (state updates not flushed), observable in Jest console
output for
this test. Wrapping the synchronous calls in waitFor/await (as suggested)
forces the calls
inside the testing-library act scope and reduces warnings/flakiness.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/SqlLab/components/EditorWrapper/EditorWrapper.test.tsx
**Line:** 153:158
**Comment:**
*Race Condition: Race condition / React update warning: calling
`onReady(mockHandle)` and `onSelectionChange(...)` synchronously can trigger
React state updates outside of the test harness' act wrapper and cause flaky
tests or warnings. Wrap these calls in `waitFor` (already imported) so they run
inside the testing-library/React act wrapper and ensure state updates are
flushed before assertions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx:
##########
@@ -216,16 +216,33 @@ 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 (backspace triggers these)
+ // but still update cache to track state
+ if (selectedText.length === 1) {
currentSelectionCache.current = selectedText;
Review Comment:
**Suggestion:** Logic bug: detecting backspace-triggered collapsed selection
by `selectedText.length === 1` is brittle and blocks actual single-character
user selections; instead detect collapsed (no-range) selections using the
`selections` start/end positions so genuine single-char selections are
captured. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Single-character selections not tracked in Redux.
- ⚠️ Selection-based features miss single-char selections.
- ⚠️ Keyboard/mouse single-char selection UX degraded.
```
</details>
```suggestion
// Skip collapsed selections (cursor movement/backspace) by checking
ranges,
// but allow real single-character selections where start !== end
if (selections.length === 1) {
const sel = selections[0];
if (
sel.start.line === sel.end.line &&
sel.start.column === sel.end.column
) {
currentSelectionCache.current = selectedText;
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a SQL editor tab (EditorWrapper at
superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx).
2. Select exactly one character in the editor (for example, click-drag a
single character
or use Shift+Arrow to select a single character).
3. EditorHost invokes onSelectionChange -> handleSelectionChange (see
handleSelectionChange at index.tsx:212-248). The code computes selectedText
via
editorHandleRef.current.getSelectedText() and then evaluates "if
(selectedText.length ===
1)" (index.tsx:234-238).
4. Because the check uses text length, the code treats genuine
single-character selections
the same as backspace-triggered collapsed events: it updates the local cache
but returns
early and does not dispatch queryEditorSetSelectedText(queryEditor,
selectedText). As a
result, single-character selections are not stored in Redux or propagated to
consumers.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/EditorWrapper/index.tsx
**Line:** 234:237
**Comment:**
*Logic Error: Logic bug: detecting backspace-triggered collapsed
selection by `selectedText.length === 1` is brittle and blocks actual
single-character user selections; instead detect collapsed (no-range)
selections using the `selections` start/end positions so genuine single-char
selections are captured.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
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();
+
+ 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();
+
+ userEvent.click(screen.getByRole('button', { name: /switch/i }));
Review Comment:
**Suggestion:** Missing await on the async `userEvent.click` call in the
cursor-position test: failing to await the click may let the test call the
cursor handler before React has applied the prop change, causing flaky
failures; change the call to `await userEvent.click(...)`. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ AceEditorProvider cursor test flakiness in CI.
- ⚠️ Intermittent CI failures for cursor-related tests.
```
</details>
```suggestion
await userEvent.click(screen.getByRole('button', { name: /switch/i }));
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/src/core/editors/AceEditorProvider.test.tsx` and
find the
cursor test `test('onCursorPositionChange uses latest callback after prop
change', ...)`
starting at line 132.
2. The test renders `CallbackSwitcher`, triggers the mocked editor load at
lines 151-155
(`mockOnLoadCallback(mockEditor)` at 153-155), and waits for cursor event
registration at
line 157: `await waitFor(() =>
expect(mockEventHandlers.changeCursor).toBeDefined());`.
3. The test simulates clicking the "Switch" button at line 164 using
`userEvent.click(...)` but does not await it, then immediately invokes
`mockEventHandlers.changeCursor!();` at line 165.
4. Because modern `userEvent.click` implementations return a Promise and
React state
updates are async, failing to `await` the click can let the handler
invocation occur
before the prop update is applied, making the assertion on which callback
ran flaky.
5. Expected fix: change the click call at line 164 to `await
userEvent.click(...)` so the
prop change is applied before invoking the mocked handler. This affects test
stability
only.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/AceEditorProvider.test.tsx
**Line:** 164:164
**Comment:**
*Race Condition: Missing await on the async `userEvent.click` call in
the cursor-position test: failing to await the click may let the test call the
cursor handler before React has applied the prop change, causing flaky
failures; change the call to `await userEvent.click(...)`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/core/editors/AceEditorProvider.tsx:
##########
@@ -249,34 +274,34 @@ const AceEditorProvider = forwardRef<EditorHandle,
EditorProps>(
});
}
- // Set up cursor position change listener
- if (onCursorPositionChange) {
- editor.selection.on('changeCursor', () => {
+ // Set up cursor position change listener using ref to avoid stale
closures
+ editor.selection.on('changeCursor', () => {
+ if (onCursorPositionChangeRef.current) {
const cursor = editor.getCursorPosition();
- onCursorPositionChange({
+ onCursorPositionChangeRef.current({
line: cursor.row,
column: cursor.column,
});
- });
- }
+ }
+ });
- // Set up selection change listener
- if (onSelectionChange) {
- editor.selection.on('changeSelection', () => {
+ // Set up selection change listener using ref to avoid stale closures
+ editor.selection.on('changeSelection', () => {
+ if (onSelectionChangeRef.current) {
const range = editor.getSelection().getRange();
- onSelectionChange([
+ onSelectionChangeRef.current([
{
start: { line: range.start.row, column: range.start.column },
end: { line: range.end.row, column: range.end.column },
},
]);
- });
- }
+ }
+ });
Review Comment:
**Suggestion:** Event listeners for `changeCursor` and `changeSelection` are
registered on the editor selection without preventing duplicates or removing
previous handlers, which can lead to multiple callbacks and memory leaks if
`onEditorLoad` is called more than once; register named handler functions,
remove any previously attached handlers on the same editor instance before
adding, and store the handler reference on the editor so it can be removed
safely. [resource leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Duplicate selection events trigger repeated updates.
- ⚠️ Memory growth if editor re-initialized repeatedly.
- ⚠️ SQL Lab selection-related features behave inconsistently.
```
</details>
```suggestion
const cursorHandler = () => {
if (onCursorPositionChangeRef.current) {
const cursor = editor.getCursorPosition();
onCursorPositionChangeRef.current({
line: cursor.row,
column: cursor.column,
});
}
};
// remove any previous handler attached to this editor instance to
avoid duplicates
const prevCursorHandler = (editor as any).__superset_cursorHandler;
if (prevCursorHandler && editor.selection.off) {
try {
editor.selection.off('changeCursor', prevCursorHandler);
} catch {}
}
(editor as any).__superset_cursorHandler = cursorHandler;
editor.selection.on('changeCursor', cursorHandler);
// Set up selection change listener using ref to avoid stale closures
const selectionHandler = () => {
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 },
},
]);
}
};
const prevSelectionHandler = (editor as
any).__superset_selectionHandler;
if (prevSelectionHandler && editor.selection.off) {
try {
editor.selection.off('changeSelection', prevSelectionHandler);
} catch {}
}
(editor as any).__superset_selectionHandler = selectionHandler;
editor.selection.on('changeSelection', selectionHandler);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset-frontend/src/core/editors/AceEditorProvider.tsx and locate
`onEditorLoad`
(callback defined at lines ~264-303). Inside it, the code attaches listeners
using
`editor.selection.on('changeCursor', ...)` and
`editor.selection.on('changeSelection',
...)` at lines ~277 and ~289.
2. Cause `onEditorLoad` to run more than once for the same editor instance
(for example:
re-initialize the EditorComponent by changing props that force the editor to
recreate, or
when the editor library triggers multiple load events). `onEditorLoad` will
attach the
same anonymous handlers again because no removal or deduplication is
performed.
3. Each subsequent `onEditorLoad` invocation adds another listener;
selection/cursor
events fire multiple times calling `onCursorPositionChangeRef.current` /
`onSelectionChangeRef.current` repeatedly.
4. Observable effects include duplicate state updates or Redux dispatches
from selection
handlers, and potential memory growth from retained closures tied to editor
instances. The
root code locations are the `editor.selection.on` calls at lines ~277-299.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/AceEditorProvider.tsx
**Line:** 278:299
**Comment:**
*Resource Leak: Event listeners for `changeCursor` and
`changeSelection` are registered on the editor selection without preventing
duplicates or removing previous handlers, which can lead to multiple callbacks
and memory leaks if `onEditorLoad` is called more than once; register named
handler functions, remove any previously attached handlers on the same editor
instance before adding, and store the handler reference on the editor so it can
be removed safely.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
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();
+
+ userEvent.click(screen.getByRole('button', { name: /switch/i }));
Review Comment:
**Suggestion:** Missing await on the async `userEvent.click` call:
`userEvent.click` returns a promise in modern @testing-library/user-event
versions, so not awaiting it can cause the component state update (which
switches the callback prop) to not be applied before the test invokes the
selection handler, introducing a race/flakiness; await the click to ensure the
state change completes before calling the handler. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ AceEditorProvider unit test flakiness in CI.
- ⚠️ Intermittent CI failures on PRs running tests.
```
</details>
```suggestion
await userEvent.click(screen.getByRole('button', { name: /switch/i }));
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file
`superset-frontend/src/core/editors/AceEditorProvider.test.tsx` and
locate the selection test `test('onSelectionChange uses latest callback
after prop
change', ...)` starting at line 93.
2. The test renders `CallbackSwitcher` and invokes the mocked editor load at
lines 112-116
where `mockOnLoadCallback(mockEditor)` is called (lines 114-116).
3. The test waits for the selection event registration at line 118: `await
waitFor(() =>
expect(mockEventHandlers.changeSelection).toBeDefined());`.
4. Later, the test triggers a UI click to swap callbacks using
`userEvent.click(...)` at
line 125 and immediately calls the mocked handler at line 126:
`mockEventHandlers.changeSelection!();`. Because `userEvent.click` can be
asynchronous in
modern @testing-library/user-event, not awaiting the click (line 125) can
allow the test
to call the handler (line 126) before React applies the new prop, causing
the old callback
to run and introducing flakiness.
5. Expected fix: await the click so React state update completes before
invoking the mock
handler. This is a test-timing issue (not runtime production code).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/AceEditorProvider.test.tsx
**Line:** 125:125
**Comment:**
*Race Condition: Missing await on the async `userEvent.click` call:
`userEvent.click` returns a promise in modern @testing-library/user-event
versions, so not awaiting it can cause the component state update (which
switches the callback prop) to not be applied before the test invokes the
selection handler, introducing a race/flakiness; await the click to ensure the
state change completes before calling the handler.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/core/editors/AceEditorProvider.tsx:
##########
@@ -222,15 +223,39 @@ 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);
+
+ // 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]);
Review Comment:
**Suggestion:** The new useEffect that calls `onReady` only runs when
`onReady` or `handle` change; it does not re-run when the editor instance
becomes available later, so if `onReady` is provided before the Ace editor is
loaded it will never be called. Add the editor reference to the effect's
dependencies so the effect re-evaluates once `aceEditorRef.current.editor` is
set (or call `onReady` from `onEditorLoad`). This ensures `onReady` is invoked
once the editor exists. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Consumer onReady callbacks not invoked after editor loads.
- ⚠️ Extensions won't receive EditorHandle for initialization.
- ⚠️ Features relying on imperative handle fail silently.
```
</details>
```suggestion
// include the editor reference so the effect re-runs when the editor
becomes available
}, [onReady, handle, aceEditorRef.current?.editor]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In superset-frontend/src/core/editors/AceEditorProvider.tsx, the
component destructures
`onReady` from props (lines ~216-219) and declares the `useEffect` that
invokes `onReady`
at lines 252-261.
2. Mount AceEditorProvider with an `onReady` callback provided by the parent
(so `onReady`
is defined before editor initialization). The `useEffect` at lines 252-261
runs after
first render while `aceEditorRef.current?.editor` is still undefined.
3. When the Ace editor instance becomes available later, `onEditorLoad`
(defined at lines
~264-303) runs to initialize the editor, but `onReady` is not called there
and the
original `useEffect` won't re-run because its dependency list is [onReady,
handle] and
does not include the editor reference.
4. Result: the parent never receives `onReady(handle)` — observed as a
missing invocation
at lines 252-261 even after editor initialization. This reproduces reliably
by rendering
AceEditorProvider with an `onReady` prop and letting the editor initialize
asynchronously
(no extra mocks needed).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/AceEditorProvider.tsx
**Line:** 261:261
**Comment:**
*Possible Bug: The new useEffect that calls `onReady` only runs when
`onReady` or `handle` change; it does not re-run when the editor instance
becomes available later, so if `onReady` is provided before the Ace editor is
loaded it will never be called. Add the editor reference to the effect's
dependencies so the effect re-evaluates once `aceEditorRef.current.editor` is
set (or call `onReady` from `onEditorLoad`). This ensures `onReady` is invoked
once the editor exists.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
