From a9c0459751d2d28bfd981846a6805bb9d6a55bdf Mon Sep 17 00:00:00 2001 From: justinpark Date: Fri, 31 May 2024 13:55:38 -0700 Subject: [PATCH] refactor(sqllab): nonblocking new query editor --- .../src/SqlLab/actions/sqlLab.js | 58 ++++------- .../src/SqlLab/actions/sqlLab.test.js | 99 +++++++++++++------ .../EditorAutoSync/EditorAutoSync.test.tsx | 34 ++++++- .../components/EditorAutoSync/index.tsx | 48 +++++++-- .../TabbedSqlEditors.test.tsx | 13 ++- .../components/TabbedSqlEditors/index.tsx | 27 ----- .../src/SqlLab/reducers/sqlLab.js | 20 +--- .../src/SqlLab/reducers/sqlLab.test.js | 28 ++++++ 8 files changed, 202 insertions(+), 125 deletions(-) diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js index 03c1855503c5d..cf74ada9a8216 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.js @@ -467,12 +467,15 @@ function migrateQuery(queryId, queryEditorId, dispatch) { ); } -export function migrateQueryEditorFromLocalStorage( - queryEditor, - tables, - queries, -) { - return function (dispatch) { +export function migrateQueryEditorFromLocalStorage(queryEditor) { + return function (dispatch, getState) { + const { tables, queries } = getState().sqlLab; + const localStorageTables = tables.filter( + table => table.inLocalStorage && table.queryEditorId === queryEditor.id, + ); + const localStorageQueries = Object.values(queries).filter( + query => query.inLocalStorage && query.sqlEditorId === queryEditor.id, + ); return SupersetClient.post({ endpoint: '/tabstateview/', postPayload: { queryEditor }, @@ -482,6 +485,7 @@ export function migrateQueryEditorFromLocalStorage( ...queryEditor, id: json.id.toString(), inLocalStorage: false, + loaded: true, }; dispatch({ type: MIGRATE_QUERY_EDITOR, @@ -494,10 +498,10 @@ export function migrateQueryEditorFromLocalStorage( newId: newQueryEditor.id, }); return Promise.all([ - ...tables.map(table => + ...localStorageTables.map(table => migrateTable(table, newQueryEditor.id, dispatch), ), - ...queries.map(query => + ...localStorageQueries.map(query => migrateQuery(query.id, newQueryEditor.id, dispatch), ), ]); @@ -516,35 +520,15 @@ export function migrateQueryEditorFromLocalStorage( } export function addQueryEditor(queryEditor) { - return function (dispatch) { - const sync = isFeatureEnabled(FeatureFlag.SqllabBackendPersistence) - ? SupersetClient.post({ - endpoint: '/tabstateview/', - postPayload: { queryEditor }, - }).then(({ json }) => ({ ...json, loaded: true })) - : Promise.resolve({ id: shortid.generate() }); - - return sync - .then(({ id, loaded }) => { - const newQueryEditor = { - ...queryEditor, - id: id.toString(), - loaded, - }; - return dispatch({ - type: ADD_QUERY_EDITOR, - queryEditor: newQueryEditor, - }); - }) - .catch(() => - dispatch( - addDangerToast( - t( - 'Unable to add a new tab to the backend. Please contact your administrator.', - ), - ), - ), - ); + const newQueryEditor = { + ...queryEditor, + id: shortid.generate().toString(), + loaded: true, + inLocalStorage: true, + }; + return { + type: ADD_QUERY_EDITOR, + queryEditor: newQueryEditor, }; } diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js b/superset-frontend/src/SqlLab/actions/sqlLab.test.js index d6b70bf6a0b40..e61fd111a5482 100644 --- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js @@ -427,13 +427,15 @@ describe('async actions', () => { maxRow: undefined, id: 'abcd', templateParams: undefined, + inLocalStorage: true, + loaded: true, }, }, ]; const request = actions.cloneQueryToNewTab(query, true); - return request(store.dispatch, store.getState).then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + request(store.dispatch, store.getState); + + expect(store.getActions()).toEqual(expectedActions); }); }); @@ -453,13 +455,16 @@ describe('async actions', () => { const expectedActions = [ { type: actions.ADD_QUERY_EDITOR, - queryEditor, + queryEditor: { + ...queryEditor, + inLocalStorage: true, + loaded: true, + }, }, ]; - const request = actions.addQueryEditor(defaultQueryEditor); - return request(store.dispatch, store.getState).then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + store.dispatch(actions.addQueryEditor(defaultQueryEditor)); + + expect(store.getActions()).toEqual(expectedActions); }); describe('addNewQueryEditor', () => { @@ -488,13 +493,14 @@ describe('async actions', () => { queryLimit: defaultQueryEditor.queryLimit || initialState.common.conf.DEFAULT_SQLLAB_LIMIT, + inLocalStorage: true, + loaded: true, }, }, ]; const request = actions.addNewQueryEditor(); - return request(store.dispatch, store.getState).then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + request(store.dispatch, store.getState); + expect(store.getActions()).toEqual(expectedActions); }); }); }); @@ -534,20 +540,33 @@ describe('async actions', () => { afterEach(fetchMock.resetHistory); describe('addQueryEditor', () => { - it('updates the tab state in the backend', () => { + let stub; + beforeEach(() => { + stub = sinon.stub(shortid, 'generate').returns('abcd'); + }); + afterEach(() => { + stub.restore(); + }); + + it('creates the tab state in the local storage', () => { expect.assertions(2); const store = mockStore({}); const expectedActions = [ { type: actions.ADD_QUERY_EDITOR, - queryEditor: { ...queryEditor, id: '1', loaded: true }, + queryEditor: { + ...queryEditor, + id: 'abcd', + loaded: true, + inLocalStorage: true, + }, }, ]; - return store.dispatch(actions.addQueryEditor(queryEditor)).then(() => { - expect(store.getActions()).toEqual(expectedActions); - expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(1); - }); + store.dispatch(actions.addQueryEditor(queryEditor)); + + expect(store.getActions()).toEqual(expectedActions); + expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(0); }); }); @@ -978,16 +997,41 @@ describe('async actions', () => { overwriteRoutes: true, }); + const oldQueryEditor = { ...queryEditor, inLocalStorage: true }; const tables = [ - { id: 'one', dataPreviewQueryId: 'previewOne' }, - { id: 'two', dataPreviewQueryId: 'previewTwo' }, + { + id: 'one', + dataPreviewQueryId: 'previewOne', + queryEditorId: oldQueryEditor.id, + inLocalStorage: true, + }, + { + id: 'two', + dataPreviewQueryId: 'previewTwo', + queryEditorId: oldQueryEditor.id, + inLocalStorage: true, + }, ]; const queries = [ - { ...query, id: 'previewOne' }, - { ...query, id: 'previewTwo' }, + { + ...query, + id: 'previewOne', + sqlEditorId: oldQueryEditor.id, + inLocalStorage: true, + }, + { + ...query, + id: 'previewTwo', + sqlEditorId: oldQueryEditor.id, + inLocalStorage: true, + }, ]; - const store = mockStore({}); - const oldQueryEditor = { ...queryEditor, inLocalStorage: true }; + const store = mockStore({ + sqlLab: { + queries, + tables, + }, + }); const expectedActions = [ { type: actions.MIGRATE_QUERY_EDITOR, @@ -997,6 +1041,7 @@ describe('async actions', () => { ...oldQueryEditor, id: '1', inLocalStorage: false, + loaded: true, }, }, { @@ -1028,13 +1073,7 @@ describe('async actions', () => { }, ]; return store - .dispatch( - actions.migrateQueryEditorFromLocalStorage( - oldQueryEditor, - tables, - queries, - ), - ) + .dispatch(actions.migrateQueryEditorFromLocalStorage(oldQueryEditor)) .then(() => { expect(store.getActions()).toEqual(expectedActions); expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(3); diff --git a/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx b/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx index 52e1d44b247f8..82d0bf561792f 100644 --- a/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx +++ b/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx @@ -40,7 +40,7 @@ import { render, waitFor } from 'spec/helpers/testing-library'; import ToastContainer from 'src/components/MessageToasts/ToastContainer'; import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; import { logging } from '@superset-ui/core'; -import EditorAutoSync from '.'; +import EditorAutoSync, { INTERVAL } from '.'; jest.mock('@superset-ui/core', () => ({ ...jest.requireActual('@superset-ui/core'), @@ -78,11 +78,37 @@ test('sync the unsaved editor tab state when there are new changes since the las sqlLab: unsavedSqlLabState, }, }); - await waitFor(() => jest.runAllTimers()); + await waitFor(() => jest.advanceTimersByTime(INTERVAL)); expect(fetchMock.calls(updateEditorTabState)).toHaveLength(1); fetchMock.restore(); }); +test('sync the unsaved NEW editor state when there are new in local storage', async () => { + const createEditorTabState = `glob:*/tabstateview/`; + fetchMock.post(createEditorTabState, { id: 123 }); + expect(fetchMock.calls(createEditorTabState)).toHaveLength(0); + render(, { + useRedux: true, + initialState: { + ...initialState, + sqlLab: { + ...initialState.sqlLab, + queryEditors: [ + ...initialState.sqlLab.queryEditors, + { + id: 'rnd-new-id', + name: 'new tab name', + inLocalStorage: true, + }, + ], + }, + }, + }); + await waitFor(() => jest.advanceTimersByTime(INTERVAL)); + expect(fetchMock.calls(createEditorTabState)).toHaveLength(1); + fetchMock.restore(); +}); + 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); @@ -102,7 +128,7 @@ test('skip syncing the unsaved editor tab state when the updates are already syn }, }, }); - await waitFor(() => jest.runAllTimers()); + await waitFor(() => jest.advanceTimersByTime(INTERVAL)); expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0); fetchMock.restore(); }); @@ -126,7 +152,7 @@ test('renders an error toast when the sync failed', async () => { }, }, ); - await waitFor(() => jest.runAllTimers()); + await waitFor(() => jest.advanceTimersByTime(INTERVAL)); expect(logging.warn).toHaveBeenCalledTimes(1); expect(logging.warn).toHaveBeenCalledWith( diff --git a/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx b/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx index 51399753e95b7..fe990a4a6424a 100644 --- a/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx +++ b/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx @@ -27,9 +27,13 @@ import { } from 'src/SqlLab/types'; import { useUpdateSqlEditorTabMutation } from 'src/hooks/apiResources/sqlEditorTabs'; import { useDebounceValue } from 'src/hooks/useDebounceValue'; -import { setEditorTabLastUpdate } from 'src/SqlLab/actions/sqlLab'; +import { + migrateQueryEditorFromLocalStorage, + setEditorTabLastUpdate, +} from 'src/SqlLab/actions/sqlLab'; +import useEffectEvent from 'src/hooks/useEffectEvent'; -const INTERVAL = 5000; +export const INTERVAL = 5000; function hasUnsavedChanges( queryEditor: QueryEditor, @@ -73,17 +77,43 @@ const EditorAutoSync: React.FC = () => { INTERVAL, ); - useEffect(() => { - const unsaved = filterUnsavedQueryEditorList( + const getUnsavedItems = useEffectEvent(unsavedQE => + filterUnsavedQueryEditorList( queryEditors, - debouncedUnsavedQueryEditor, + unsavedQE, lastSavedTimestampRef.current, - ); + ), + ); + + const getUnsavedNewQueryEditor = useEffectEvent(() => + filterUnsavedQueryEditorList( + queryEditors, + unsavedQueryEditor, + lastSavedTimestampRef.current, + ).find(({ inLocalStorage }) => Boolean(inLocalStorage)), + ); + + useEffect(() => { + let timer: NodeJS.Timeout; + function saveUnsavedQueryEditor() { + const firstUnsavedQueryEditor = getUnsavedNewQueryEditor(); + + if (firstUnsavedQueryEditor) { + dispatch(migrateQueryEditorFromLocalStorage(firstUnsavedQueryEditor)); + } + timer = setTimeout(saveUnsavedQueryEditor, INTERVAL); + } + timer = setTimeout(saveUnsavedQueryEditor, INTERVAL); + return () => { + clearTimeout(timer); + }; + }, [getUnsavedNewQueryEditor, dispatch]); + + useEffect(() => { + const unsaved = getUnsavedItems(debouncedUnsavedQueryEditor); Promise.all( unsaved - // TODO: Migrate migrateQueryEditorFromLocalStorage - // in TabbedSqlEditors logic by addSqlEditor mutation later .filter(({ inLocalStorage }) => !inLocalStorage) .map(queryEditor => updateSqlEditor({ queryEditor })), ).then(resolvers => { @@ -92,7 +122,7 @@ const EditorAutoSync: React.FC = () => { dispatch(setEditorTabLastUpdate(lastSavedTimestampRef.current)); } }); - }, [debouncedUnsavedQueryEditor, dispatch, queryEditors, updateSqlEditor]); + }, [debouncedUnsavedQueryEditor, getUnsavedItems, dispatch, updateSqlEditor]); useEffect(() => { if (error) { diff --git a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsx b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsx index 6b048830e83c2..577b13c995a9f 100644 --- a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsx +++ b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsx @@ -165,8 +165,8 @@ test('should disable new tab when offline', () => { }); expect(queryAllByLabelText('Add tab').length).toEqual(0); }); -test('should have an empty state when query editors is empty', () => { - const { getByText } = setup(undefined, { +test('should have an empty state when query editors is empty', async () => { + const { getByText, getByRole } = setup(undefined, { ...initialState, sqlLab: { ...initialState.sqlLab, @@ -174,5 +174,12 @@ test('should have an empty state when query editors is empty', () => { tabHistory: [], }, }); - expect(getByText('Add a new tab to create SQL Query')).toBeInTheDocument(); + + // Clear the new tab applied in componentDidMount and check the state of the empty tab + const removeTabButton = getByRole('button', { name: 'remove' }); + fireEvent.click(removeTabButton); + + await waitFor(() => + expect(getByText('Add a new tab to create SQL Query')).toBeInTheDocument(), + ); }); diff --git a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx index 7b4db1cbe844c..632cadc17f3d7 100644 --- a/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx +++ b/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.tsx @@ -70,32 +70,6 @@ class TabbedSqlEditors extends React.PureComponent { } componentDidMount() { - // migrate query editor and associated tables state to server - if (isFeatureEnabled(FeatureFlag.SqllabBackendPersistence)) { - const localStorageTables = this.props.tables.filter( - table => table.inLocalStorage, - ); - const localStorageQueries = Object.values(this.props.queries).filter( - query => query.inLocalStorage, - ); - this.props.queryEditors - .filter(qe => qe.inLocalStorage) - .forEach(qe => { - // get all queries associated with the query editor - const queries = localStorageQueries.filter( - query => query.sqlEditorId === qe.id, - ); - const tables = localStorageTables.filter( - table => table.queryEditorId === qe.id, - ); - this.props.actions.migrateQueryEditorFromLocalStorage( - qe, - tables, - queries, - ); - }); - } - // merge post form data with GET search params // Hack: this data should be coming from getInitialState // but for some reason this data isn't being passed properly through @@ -322,7 +296,6 @@ export function mapStateToProps({ sqlLab, common }: SqlLabRootState) { queryEditors: sqlLab.queryEditors ?? DEFAULT_PROPS.queryEditors, queries: sqlLab.queries, tabHistory: sqlLab.tabHistory, - tables: sqlLab.tables, defaultDbId: common.conf.SQLLAB_DEFAULT_DBID, displayLimit: common.conf.DISPLAY_MAX_ROW, offline: sqlLab.offline ?? DEFAULT_PROPS.offline, diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js b/superset-frontend/src/SqlLab/reducers/sqlLab.js index ecc0f090a9aaf..24acc721c32aa 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js @@ -442,9 +442,10 @@ export default function sqlLabReducer(state = {}, action) { // continue regardless of error } // replace localStorage query editor with the server backed one - return addToArr( - removeFromArr(state, 'queryEditors', action.oldQueryEditor), + return alterInArr( + state, 'queryEditors', + action.oldQueryEditor, action.newQueryEditor, ); }, @@ -468,20 +469,9 @@ export default function sqlLabReducer(state = {}, action) { ); }, [actions.MIGRATE_TAB_HISTORY]() { - try { - // remove migrated tab from localStorage tabHistory - const { sqlLab } = JSON.parse(localStorage.getItem('redux')); - sqlLab.tabHistory = sqlLab.tabHistory.filter( - tabId => tabId !== action.oldId, - ); - localStorage.setItem('redux', JSON.stringify({ sqlLab })); - } catch (error) { - // continue regardless of error - } - const tabHistory = state.tabHistory.filter( - tabId => tabId !== action.oldId, + const tabHistory = state.tabHistory.map(tabId => + tabId === action.oldId ? action.newId : tabId, ); - tabHistory.push(action.newId); return { ...state, tabHistory }; }, [actions.MIGRATE_QUERY]() { diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js index 87e0212b16424..0cf6b1d487c9a 100644 --- a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js +++ b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js @@ -239,6 +239,34 @@ describe('sqlLabReducer', () => { interceptedAction.northPercent, ); }); + it('should migrate query editor by new query editor id', () => { + const index = newState.queryEditors.findIndex(({ id }) => id === qe.id); + const newQueryEditor = { + ...qe, + id: 'updatedNewId', + schema: 'updatedSchema', + }; + const action = { + type: actions.MIGRATE_QUERY_EDITOR, + oldQueryEditor: qe, + newQueryEditor, + }; + newState = sqlLabReducer(newState, action); + expect(newState.queryEditors[index].id).toEqual('updatedNewId'); + expect(newState.queryEditors[index]).toEqual(newQueryEditor); + }); + it('should migrate tab history by new query editor id', () => { + expect(newState.tabHistory).toContain(qe.id); + const action = { + type: actions.MIGRATE_TAB_HISTORY, + oldId: qe.id, + newId: 'updatedNewId', + }; + newState = sqlLabReducer(newState, action); + + expect(newState.tabHistory).toContain('updatedNewId'); + expect(newState.tabHistory).not.toContain(qe.id); + }); }); describe('Tables', () => { let newState;