Skip to content

Commit

Permalink
refactor(sqllab): nonblocking new query editor
Browse files Browse the repository at this point in the history
  • Loading branch information
@justinpark
justinpark committed May 31, 2024
1 parent 0070097 commit a9c0459
Show file tree
Hide file tree
Showing 8 changed files with 202 additions and 125 deletions.
58 changes: 21 additions & 37 deletions superset-frontend/src/SqlLab/actions/sqlLab.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -482,6 +485,7 @@ export function migrateQueryEditorFromLocalStorage(
...queryEditor,
id: json.id.toString(),
inLocalStorage: false,
loaded: true,
};
dispatch({
type: MIGRATE_QUERY_EDITOR,
Expand All @@ -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),
),
]);
Expand All @@ -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,
};
}

Expand Down
99 changes: 69 additions & 30 deletions superset-frontend/src/SqlLab/actions/sqlLab.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand All @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
});
Expand Down Expand Up @@ -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);
});
});

Expand Down Expand Up @@ -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,
Expand All @@ -997,6 +1041,7 @@ describe('async actions', () => {
...oldQueryEditor,
id: '1',
inLocalStorage: false,
loaded: true,
},
},
{
Expand Down Expand Up @@ -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);
Expand Down
34 changes: 30 additions & 4 deletions superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down Expand Up @@ -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(<EditorAutoSync />, {
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);
Expand All @@ -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();
});
Expand All @@ -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(
Expand Down
48 changes: 39 additions & 9 deletions superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 => {
Expand All @@ -92,7 +122,7 @@ const EditorAutoSync: React.FC = () => {
dispatch(setEditorTabLastUpdate(lastSavedTimestampRef.current));
}
});
}, [debouncedUnsavedQueryEditor, dispatch, queryEditors, updateSqlEditor]);
}, [debouncedUnsavedQueryEditor, getUnsavedItems, dispatch, updateSqlEditor]);

useEffect(() => {
if (error) {
Expand Down
Loading

0 comments on commit a9c0459

Please sign in to comment.