refactor(sqllab): nonblocking delete query editor (#29233)

apache · Jun 14, 2024 · ddc9f06 · ddc9f06
1 parent 7ddea62
commit ddc9f06
Show file tree

Hide file tree

Showing 13 changed files with 155 additions and 32 deletions.
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js
@@ -103,6 +103,7 @@ export const CREATE_DATASOURCE_FAILED = 'CREATE_DATASOURCE_FAILED';
 
 export const SET_EDITOR_TAB_LAST_UPDATE = 'SET_EDITOR_TAB_LAST_UPDATE';
 export const SET_LAST_UPDATED_ACTIVE_TAB = 'SET_LAST_UPDATED_ACTIVE_TAB';
+export const CLEAR_DESTROYED_QUERY_EDITOR = 'CLEAR_DESTROYED_QUERY_EDITOR';
 
 export const addInfoToast = addInfoToastAction;
 export const addSuccessToast = addSuccessToastAction;
@@ -715,29 +716,12 @@ export function toggleLeftBar(queryEditor) {
   };
 }
 
-export function removeQueryEditor(queryEditor) {
-  return function (dispatch) {
-    const sync = isFeatureEnabled(FeatureFlag.SqllabBackendPersistence)
-      ? SupersetClient.delete({
-          endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
-        })
-      : Promise.resolve();
+export function clearDestoryedQueryEditor(queryEditorId) {
+  return { type: CLEAR_DESTROYED_QUERY_EDITOR, queryEditorId };
+}
 
-    return sync
-      .then(() => dispatch({ type: REMOVE_QUERY_EDITOR, queryEditor }))
-      .catch(({ status }) => {
-        if (status !== 404) {
-          return dispatch(
-            addDangerToast(
-              t(
-                'An error occurred while removing tab. Please contact your administrator.',
-              ),
-            ),
-          );
-        }
-        return dispatch({ type: REMOVE_QUERY_EDITOR, queryEditor });
-      });
-  };
+export function removeQueryEditor(queryEditor) {
+  return { type: REMOVE_QUERY_EDITOR, queryEditor };
 }
 
 export function removeAllOtherQueryEditors(queryEditor) {

diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js
@@ -610,7 +610,7 @@ describe('async actions', () => {
 
     describe('removeQueryEditor', () => {
       it('updates the tab state in the backend', () => {
-        expect.assertions(2);
+        expect.assertions(1);
 
         const store = mockStore({});
         const expectedActions = [
@@ -619,12 +619,8 @@ describe('async actions', () => {
             queryEditor,
           },
         ];
-        return store
-          .dispatch(actions.removeQueryEditor(queryEditor))
-          .then(() => {
-            expect(store.getActions()).toEqual(expectedActions);
-            expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(1);
-          });
+        store.dispatch(actions.removeQueryEditor(queryEditor));
+        expect(store.getActions()).toEqual(expectedActions);
       });
     });
 

diff --git a/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx b/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
@@ -146,6 +146,29 @@ test('sync the active editor id when there are updates in tab history', async ()
   expect(fetchMock.calls(updateActiveEditorTabState)).toHaveLength(1);
 });
 
+test('sync the destroyed editor id when there are updates in destroyed editors', async () => {
+  const removeId = 'removed-tab-id';
+  const deleteEditorState = `glob:*/tabstateview/${removeId}`;
+  fetchMock.delete(deleteEditorState, { id: removeId });
+  expect(fetchMock.calls(deleteEditorState)).toHaveLength(0);
+  render(<EditorAutoSync />, {
+    useRedux: true,
+    initialState: {
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        destroyedQueryEditors: {
+          [removeId]: 123,
+        },
+      },
+    },
+  });
+  await waitFor(() => jest.advanceTimersByTime(INTERVAL));
+  expect(fetchMock.calls(deleteEditorState)).toHaveLength(1);
+  await waitFor(() => jest.advanceTimersByTime(INTERVAL));
+  expect(fetchMock.calls(deleteEditorState)).toHaveLength(1);
+});
+
 test('skip syncing the unsaved editor tab state when the updates are already synced', async () => {
   const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
   fetchMock.put(updateEditorTabState, 200);

diff --git a/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx b/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
@@ -29,12 +29,14 @@ import {
 import {
   useUpdateCurrentSqlEditorTabMutation,
   useUpdateSqlEditorTabMutation,
+  useDeleteSqlEditorTabMutation,
 } from 'src/hooks/apiResources/sqlEditorTabs';
 import { useDebounceValue } from 'src/hooks/useDebounceValue';
 import {
   syncQueryEditor,
   setEditorTabLastUpdate,
   setLastUpdatedActiveTab,
+  clearDestoryedQueryEditor,
 } from 'src/SqlLab/actions/sqlLab';
 import useEffectEvent from 'src/hooks/useEffectEvent';
 
@@ -82,8 +84,13 @@ const EditorAutoSync: FC = () => {
   const lastUpdatedActiveTab = useSelector<SqlLabRootState, string>(
     ({ sqlLab }) => sqlLab.lastUpdatedActiveTab,
   );
+  const destroyedQueryEditors = useSelector<
+    SqlLabRootState,
+    Record<string, number>
+  >(({ sqlLab }) => sqlLab.destroyedQueryEditors);
   const [updateSqlEditor, { error }] = useUpdateSqlEditorTabMutation();
   const [updateCurrentSqlEditor] = useUpdateCurrentSqlEditorTabMutation();
+  const [deleteSqlEditor] = useDeleteSqlEditorTabMutation();
 
   const debouncedUnsavedQueryEditor = useDebounceValue(
     unsavedQueryEditor,
@@ -119,6 +126,22 @@ const EditorAutoSync: FC = () => {
     }
   });
 
+  const syncDeletedQueryEditor = useEffectEvent(() => {
+    if (Object.keys(destroyedQueryEditors).length > 0) {
+      Object.keys(destroyedQueryEditors).forEach(id => {
+        deleteSqlEditor(id)
+          .then(() => {
+            dispatch(clearDestoryedQueryEditor(id));
+          })
+          .catch(({ status }) => {
+            if (status === 404) {
+              dispatch(clearDestoryedQueryEditor(id));
+            }
+          });
+      });
+    }
+  });
+
   useEffect(() => {
     let saveTimer: NodeJS.Timeout;
     function saveUnsavedQueryEditor() {
@@ -131,11 +154,18 @@ const EditorAutoSync: FC = () => {
     }
     const syncTimer = setInterval(syncCurrentQueryEditor, INTERVAL);
     saveTimer = setTimeout(saveUnsavedQueryEditor, INTERVAL);
+    const clearQueryEditorTimer = setInterval(syncDeletedQueryEditor, INTERVAL);
     return () => {
       clearTimeout(saveTimer);
       clearInterval(syncTimer);
+      clearInterval(clearQueryEditorTimer);
     };
-  }, [getUnsavedNewQueryEditor, syncCurrentQueryEditor, dispatch]);
+  }, [
+    getUnsavedNewQueryEditor,
+    syncCurrentQueryEditor,
+    syncDeletedQueryEditor,
+    dispatch,
+  ]);
 
   useEffect(() => {
     const unsaved = getUnsavedItems(debouncedUnsavedQueryEditor);

diff --git a/superset-frontend/src/SqlLab/fixtures.ts b/superset-frontend/src/SqlLab/fixtures.ts
@@ -681,6 +681,7 @@ export const initialState = {
     queriesLastUpdate: 0,
     activeSouthPaneTab: 'Results',
     unsavedQueryEditor: {},
+    destroyedQueryEditors: {},
   },
   messageToasts: [],
   user,

diff --git a/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js b/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js
@@ -50,6 +50,7 @@ const sqlLabPersistStateConfig = {
             queries,
             tabHistory,
             lastUpdatedActiveTab,
+            destroyedQueryEditors,
           } = state.sqlLab;
           const unsavedQueryEditors = filterUnsavedQueryEditorList(
             queryEditors,
@@ -58,7 +59,13 @@ const sqlLabPersistStateConfig = {
           );
           const hasUnsavedActiveTabState =
             tabHistory.slice(-1)[0] !== lastUpdatedActiveTab;
-          if (unsavedQueryEditors.length > 0 || hasUnsavedActiveTabState) {
+          const hasUnsavedDeletedQueryEditors =
+            Object.keys(destroyedQueryEditors).length > 0;
+          if (
+            unsavedQueryEditors.length > 0 ||
+            hasUnsavedActiveTabState ||
+            hasUnsavedDeletedQueryEditors
+          ) {
             const hasFinishedMigrationFromLocalStorage =
               unsavedQueryEditors.every(
                 ({ inLocalStorage }) => !inLocalStorage,
@@ -76,6 +83,7 @@ const sqlLabPersistStateConfig = {
               ...(hasUnsavedActiveTabState && {
                 tabHistory,
               }),
+              destroyedQueryEditors,
             };
           }
           return;

diff --git a/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts b/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts
@@ -274,6 +274,9 @@ describe('getInitialState', () => {
                 name: expectedValue,
               },
             ],
+            destroyedQueryEditors: {
+              10: 12345,
+            },
           },
         }),
       );
@@ -291,7 +294,10 @@ describe('getInitialState', () => {
             updatedAt: lastUpdatedTime,
           },
         },
-        tab_state_ids: [{ id: 1, label: '' }],
+        tab_state_ids: [
+          { id: 1, label: '' },
+          { id: 10, label: 'removed' },
+        ],
       };
       expect(
         getInitialState(apiDataWithLocalStorage).sqlLab.queryEditors[0],
@@ -301,6 +307,13 @@ describe('getInitialState', () => {
           name: expectedValue,
         }),
       );
+      expect(
+        getInitialState(apiDataWithLocalStorage).sqlLab.queryEditors,
+      ).not.toContainEqual(
+        expect.objectContaining({
+          id: '10',
+        }),
+      );
       expect(
         getInitialState(apiDataWithLocalStorage).sqlLab.lastUpdatedActiveTab,
       ).toEqual(apiDataWithTabState.active_tab.id.toString());

diff --git a/superset-frontend/src/SqlLab/reducers/getInitialState.ts b/superset-frontend/src/SqlLab/reducers/getInitialState.ts
@@ -146,6 +146,8 @@ export default function getInitialState({
     }),
   };
 
+  const destroyedQueryEditors = {};
+
   /**
    * If the `SQLLAB_BACKEND_PERSISTENCE` feature flag is off, or if the user
    * hasn't used SQL Lab after it has been turned on, the state will be stored
@@ -219,6 +221,15 @@ export default function getInitialState({
           tabHistory.push(...sqlLab.tabHistory);
         }
         lastUpdatedActiveTab = tabHistory.slice(tabHistory.length - 1)[0] || '';
+
+        if (sqlLab.destroyedQueryEditors) {
+          Object.entries(sqlLab.destroyedQueryEditors).forEach(([id, ts]) => {
+            if (queryEditors[id]) {
+              destroyedQueryEditors[id] = ts;
+              delete queryEditors[id];
+            }
+          });
+        }
       }
     }
   } catch (error) {
@@ -253,6 +264,7 @@ export default function getInitialState({
       queryCostEstimates: {},
       unsavedQueryEditor,
       lastUpdatedActiveTab,
+      destroyedQueryEditors,
     },
     localStorageUsageInKilobytes: 0,
     common,

diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js
@@ -163,9 +163,18 @@ export default function sqlLabReducer(state = {}, action) {
           ...(action.queryEditor.id !== state.unsavedQueryEditor.id &&
             state.unsavedQueryEditor),
         },
+        destroyedQueryEditors: {
+          ...newState.destroyedQueryEditors,
+          [queryEditor.id]: Date.now(),
+        },
       };
       return newState;
     },
+    [actions.CLEAR_DESTROYED_QUERY_EDITOR]() {
+      const destroyedQueryEditors = { ...state.destroyedQueryEditors };
+      delete destroyedQueryEditors[action.queryEditorId];
+      return { ...state, destroyedQueryEditors };
+    },
     [actions.REMOVE_QUERY]() {
       const newQueries = { ...state.queries };
       delete newQueries[action.query.id];

diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js
@@ -267,6 +267,23 @@ describe('sqlLabReducer', () => {
       expect(newState.tabHistory).toContain('updatedNewId');
       expect(newState.tabHistory).not.toContain(qe.id);
     });
+    it('should clear the destroyed query editors', () => {
+      const expectedQEId = '1233289';
+      const action = {
+        type: actions.CLEAR_DESTROYED_QUERY_EDITOR,
+        queryEditorId: expectedQEId,
+      };
+      newState = sqlLabReducer(
+        {
+          ...newState,
+          destroyedQueryEditors: {
+            [expectedQEId]: Date.now(),
+          },
+        },
+        action,
+      );
+      expect(newState.destroyedQueryEditors).toEqual({});
+    });
   });
   describe('Tables', () => {
     let newState;

diff --git a/superset-frontend/src/SqlLab/types.ts b/superset-frontend/src/SqlLab/types.ts
@@ -110,6 +110,7 @@ export type SqlLabRootState = {
     queryCostEstimates?: Record<string, QueryCostEstimate>;
     editorTabLastUpdatedAt: number;
     lastUpdatedActiveTab: string;
+    destroyedQueryEditors: Record<string, number>;
   };
   localStorageUsageInKilobytes: number;
   messageToasts: toastState[];

diff --git a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts
@@ -25,6 +25,7 @@ import {
 import { api } from 'src/hooks/apiResources/queryApi';
 import { LatestQueryEditorVersion } from 'src/SqlLab/types';
 import {
+  useDeleteSqlEditorTabMutation,
   useUpdateCurrentSqlEditorTabMutation,
   useUpdateSqlEditorTabMutation,
 } from './sqlEditorTabs';
@@ -120,3 +121,23 @@ test('posts activate request with queryEditorId', async () => {
     expect(fetchMock.calls(tabStateMutationApiRoute).length).toBe(1),
   );
 });
+
+test('deletes destoryed query editors', async () => {
+  const tabStateMutationApiRoute = `glob:*/tabstateview/${expectedQueryEditor.id}`;
+  fetchMock.delete(tabStateMutationApiRoute, 200);
+  const { result, waitFor } = renderHook(
+    () => useDeleteSqlEditorTabMutation(),
+    {
+      wrapper: createWrapper({
+        useRedux: true,
+        store,
+      }),
+    },
+  );
+  act(() => {
+    result.current[0](expectedQueryEditor.id);
+  });
+  await waitFor(() =>
+    expect(fetchMock.calls(tabStateMutationApiRoute).length).toBe(1),
+  );
+});
diff --git a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts
@@ -73,10 +73,18 @@ const sqlEditorApi = api.injectEndpoints({
         transformResponse: () => queryEditorId,
       }),
     }),
+    deleteSqlEditorTab: builder.mutation<void, string>({
+      query: queryEditorId => ({
+        method: 'DELETE',
+        endpoint: encodeURI(`/tabstateview/${queryEditorId}`),
+        transformResponse: () => queryEditorId,
+      }),
+    }),
   }),
 });
 
 export const {
   useUpdateSqlEditorTabMutation,
   useUpdateCurrentSqlEditorTabMutation,
+  useDeleteSqlEditorTabMutation,
 } = sqlEditorApi;