fix(sqllab): race condition when updating cursor position (#30154)

Author: justinpark

superset-frontend/src/SqlLab/components/AceEditorWrapper/AceEditorWrapper.test.tsx

@@ -18,16 +18,29 @@
  */
 import configureStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
-import { render, waitFor } from 'spec/helpers/testing-library';
+import reducerIndex from 'spec/helpers/reducerIndex';
+import { render, waitFor, createStore } from 'spec/helpers/testing-library';
 import { QueryEditor } from 'src/SqlLab/types';
 import { Store } from 'redux';
 import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
 import AceEditorWrapper from 'src/SqlLab/components/AceEditorWrapper';
-import { AsyncAceEditorProps } from 'src/components/AsyncAceEditor';
+import {
+  AsyncAceEditorProps,
+  FullSQLEditor,
+} from 'src/components/AsyncAceEditor';
+import {
+  queryEditorSetCursorPosition,
+  queryEditorSetDb,
+} from 'src/SqlLab/actions/sqlLab';
+import fetchMock from 'fetch-mock';
 
 const middlewares = [thunk];
 const mockStore = configureStore(middlewares);
 
+fetchMock.get('glob:*/api/v1/database/*/function_names/', {
+  function_names: [],
+});
+
 jest.mock('src/components/Select/Select', () => () => (
   <div data-test="mock-deprecated-select-select" />
 ));
@@ -36,9 +49,11 @@ jest.mock('src/components/Select/AsyncSelect', () => () => (
 ));
 
 jest.mock('src/components/AsyncAceEditor', () => ({
-  FullSQLEditor: (props: AsyncAceEditorProps) => (
-    <div data-test="react-ace">{JSON.stringify(props)}</div>
-  ),
+  FullSQLEditor: jest
+    .fn()
+    .mockImplementation((props: AsyncAceEditorProps) => (
+      <div data-test="react-ace">{JSON.stringify(props)}</div>
+    )),
 }));
 
 const setup = (queryEditor: QueryEditor, store?: Store) =>
@@ -59,6 +74,10 @@ const setup = (queryEditor: QueryEditor, store?: Store) =>
   );
 
 describe('AceEditorWrapper', () => {
+  beforeEach(() => {
+    (FullSQLEditor as any as jest.Mock).mockClear();
+  });
+
   it('renders ace editor including sql value', async () => {
     const { getByTestId } = setup(defaultQueryEditor, mockStore(initialState));
     await waitFor(() => expect(getByTestId('react-ace')).toBeInTheDocument());
@@ -91,4 +110,19 @@ describe('AceEditorWrapper', () => {
       JSON.stringify({ value: defaultQueryEditor.sql }).slice(1, -1),
     );
   });
+
+  it('skips rerendering for updating cursor position', () => {
+    const store = createStore(initialState, reducerIndex);
+    setup(defaultQueryEditor, store);
+
+    expect(FullSQLEditor).toHaveBeenCalled();
+    const renderCount = (FullSQLEditor as any as jest.Mock).mock.calls.length;
+    const updatedCursorPosition = { row: 1, column: 9 };
+    store.dispatch(
+      queryEditorSetCursorPosition(defaultQueryEditor, updatedCursorPosition),
+    );
+    expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount);
+    store.dispatch(queryEditorSetDb(defaultQueryEditor, 1));
+    expect(FullSQLEditor).toHaveBeenCalledTimes(renderCount + 1);
+  });
 });

superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx

@@ -18,7 +18,7 @@
  */
 import { useState, useEffect, useRef } from 'react';
 import type { IAceEditor } from 'react-ace/lib/types';
-import { useDispatch } from 'react-redux';
+import { shallowEqual, useDispatch, useSelector } from 'react-redux';
 import { css, styled, usePrevious, useTheme } from '@superset-ui/core';
 import { Global } from '@emotion/react';
 
@@ -27,7 +27,7 @@ import { queryEditorSetSelectedText } from 'src/SqlLab/actions/sqlLab';
 import { FullSQLEditor as AceEditor } from 'src/components/AsyncAceEditor';
 import type { KeyboardShortcut } from 'src/SqlLab/components/KeyboardShortcutButton';
 import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
-import type { CursorPosition } from 'src/SqlLab/types';
+import { SqlLabRootState, type CursorPosition } from 'src/SqlLab/types';
 import { useAnnotations } from './useAnnotations';
 import { useKeywords } from './useKeywords';
 
@@ -77,11 +77,20 @@ const AceEditorWrapper = ({
     'catalog',
     'schema',
     'templateParams',
-    'cursorPosition',
   ]);
+  // Prevent a maximum update depth exceeded error
+  // by skipping access the unsaved query editor state
+  const cursorPosition = useSelector<SqlLabRootState, CursorPosition>(
+    ({ sqlLab: { queryEditors } }) => {
+      const { cursorPosition } = {
+        ...queryEditors.find(({ id }) => id === queryEditorId),
+      };
+      return cursorPosition ?? { row: 0, column: 0 };
+    },
+    shallowEqual,
+  );
 
   const currentSql = queryEditor.sql ?? '';
-  const cursorPosition = queryEditor.cursorPosition ?? { row: 0, column: 0 };
   const [sql, setSql] = useState(currentSql);
 
   // The editor changeSelection is called multiple times in a row,
@@ -145,12 +154,7 @@ const AceEditorWrapper = ({
 
     editor.selection.on('changeCursor', () => {
       const cursor = editor.getCursorPosition();
-      const { row, column } = cursorPosition;
-      // Prevent a maximum update depth exceeded error
-      // by skipping repeated updates to the same cursor position
-      if (cursor.row !== row && cursor.column !== column) {
-        onCursorPositionChange(cursor);
-      }
+      onCursorPositionChange(cursor);
     });
 
     const { row, column } = cursorPosition;

superset-frontend/src/SqlLab/components/AceEditorWrapper/useAnnotations.ts

@@ -24,9 +24,12 @@ import { VALIDATION_DEBOUNCE_MS } from 'src/SqlLab/constants';
 import {
   FetchValidationQueryParams,
   useQueryValidationsQuery,
+  ValidationResult,
 } from 'src/hooks/apiResources';
 import { useDebounceValue } from 'src/hooks/useDebounceValue';
 
+const EMPTY = [] as ValidationResult[];
+
 export function useAnnotations(params: FetchValidationQueryParams) {
   const { sql, dbId, schema, templateParams } = params;
   const debouncedSql = useDebounceValue(sql, VALIDATION_DEBOUNCE_MS);
@@ -73,7 +76,7 @@ export function useAnnotations(params: FetchValidationQueryParams) {
                       text: `The server failed to validate your query.\n${message}`,
                     },
                   ]
-                : [],
+                : EMPTY,
         };
       },
     },

superset-frontend/src/hooks/apiResources/queryValidations.ts

@@ -26,7 +26,7 @@ export type FetchValidationQueryParams = {
   templateParams?: string;
 };
 
-type ValidationResult = {
+export type ValidationResult = {
   end_column: number | null;
   line_number: number | null;
   message: string | null;