Skip to content

Commit

Permalink
Optimize query editor: handle query text changes faster
Browse files Browse the repository at this point in the history
  • Loading branch information
kravets-levko committed Jan 2, 2020
1 parent 0c1b45f commit 2934e53
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 21 deletions.
14 changes: 5 additions & 9 deletions client/app/pages/queries/QuerySource.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { isEmpty, find, map, extend, includes } from "lodash";
import React, { useState, useRef, useEffect, useCallback } from "react";
import PropTypes from "prop-types";
import { react2angular } from "react2angular";
import { useDebouncedCallback } from "use-debounce";
import Select from "antd/lib/select";
import Resizable from "@/components/Resizable";
import { Parameters } from "@/components/Parameters";
Expand Down Expand Up @@ -51,7 +50,7 @@ function chooseDataSourceId(dataSourceIds, availableDataSources) {
}

function QuerySource(props) {
const { query, setQuery, isDirty, saveQuery } = useQuery(props.query);
const { query, setQuery, querySource, setQuerySource, isDirty, saveQuery } = useQuery(props.query);
const { dataSourcesLoaded, dataSources, dataSource } = useQueryDataSources(query);
const [schema, refreshSchema] = useDataSourceSchema(dataSource);
const queryFlags = useQueryFlags(query, dataSource);
Expand All @@ -73,10 +72,6 @@ function QuerySource(props) {
const editorRef = useRef(null);
const [autocompleteAvailable, autocompleteEnabled, toggleAutocomplete] = useAutocompleteFlags(schema);

const [handleQueryEditorChange] = useDebouncedCallback(queryText => {
setQuery(extend(query.clone(), { query: queryText }));
}, 200);

useEffect(() => {
recordEvent("view_source", "query", query.id);
}, [query.id]);
Expand Down Expand Up @@ -129,11 +124,12 @@ function QuerySource(props) {
const openEmbedDialog = useEmbedDialog(query);
const editSchedule = useEditScheduleDialog(query, setQuery);
const openAddNewParameterDialog = useAddNewParameterDialog(query, (newQuery, param) => {
// order is important - first update entire query object, and only then - paste snippet
setQuery(newQuery);
if (editorRef.current) {
editorRef.current.paste(param.toQueryTextFragment());
editorRef.current.focus();
}
setQuery(newQuery);
});

const addVisualization = useEditVisualizationDialog(query, queryResult, (newQuery, visualization) => {
Expand Down Expand Up @@ -251,10 +247,10 @@ function QuerySource(props) {
ref={editorRef}
data-executing={isQueryExecuting ? "true" : null}
syntax={dataSource ? dataSource.syntax : null}
value={query.query}
value={querySource}
schema={schema}
autocompleteEnabled={autocompleteAvailable && autocompleteEnabled}
onChange={handleQueryEditorChange}
onChange={setQuerySource}
onSelectionChange={setSelectedText}
/>

Expand Down
19 changes: 17 additions & 2 deletions client/app/pages/queries/hooks/useQuery.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { useState, useMemo } from "react";
import { useState, useMemo, useEffect } from "react";
import useUpdateQuery from "./useUpdateQuery";
import navigateTo from "@/services/navigateTo";

export default function useQuery(originalQuery) {
const [query, setQuery] = useState(originalQuery);
const [originalQuerySource, setOriginalQuerySource] = useState(originalQuery.query);

// Editor may change query text very frequently, so use separate state variable
// to reduce amount of component updates
const [querySource, setQuerySource] = useState(originalQuery.query);

const updateQuery = useUpdateQuery(query, updatedQuery => {
// It's important to update URL first, and only then update state
if (updatedQuery.id !== query.id) {
Expand All @@ -16,13 +20,24 @@ export default function useQuery(originalQuery) {
setOriginalQuerySource(updatedQuery.query);
});

useEffect(() => {
setQuerySource(query.query);
}, [query]);

return useMemo(
() => ({
query,
setQuery,
querySource,
setQuerySource: source => {
// order is important: mutate before state update; state change will immediately
// re-render components, so query object should be in sync with state
query.query = source;
setQuerySource(source);
},
isDirty: query.query !== originalQuerySource,
saveQuery: () => updateQuery(),
}),
[query, originalQuerySource, updateQuery]
[query, querySource, originalQuerySource, updateQuery]
);
}
2 changes: 1 addition & 1 deletion client/app/pages/queries/hooks/useQueryFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ export default function useQueryFlags(query, dataSource = null) {
canFork: currentUser.hasPermission("edit_query") && !dataSource.view_only,
canSchedule: currentUser.hasPermission("schedule_query"),
}),
[query, dataSource.view_only]
[query.id, query.is_draft, query.is_archived, query.can_edit, query.is_safe, query.query, dataSource.view_only]
);
}
10 changes: 7 additions & 3 deletions client/app/pages/queries/hooks/useQueryParameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,27 @@ import { isUndefined } from "lodash";
import { useEffect, useMemo, useState, useCallback } from "react";

export default function useQueryParameters(query) {
const parameters = useMemo(() => query.getParametersDefs(), [query]);
// query.getParametersDefs() implicitly depends on query.query
// eslint-disable-next-line react-hooks/exhaustive-deps
const parameters = useMemo(() => query.getParametersDefs(), [query, query.query]);

const [dirtyFlag, setDirtyFlag] = useState(query.getParameters().hasPendingValues());

const updateDirtyFlag = useCallback(
flag => {
flag = isUndefined(flag) ? query.getParameters().hasPendingValues() : flag;
setDirtyFlag(flag);
},
[query]
// eslint-disable-next-line react-hooks/exhaustive-deps
[query, query.query] // query.getParameters() implicitly depends on query.query
);

useEffect(() => {
const updatedDirtyParameters = query.getParameters().hasPendingValues();
if (updatedDirtyParameters !== dirtyFlag) {
setDirtyFlag(updatedDirtyParameters);
}
}, [query, parameters, dirtyFlag]);
}, [query, query.query, parameters, dirtyFlag]); // query.getParameters() implicitly depends on query.query

return useMemo(() => [parameters, dirtyFlag, updateDirtyFlag], [parameters, dirtyFlag, updateDirtyFlag]);
}
4 changes: 0 additions & 4 deletions client/cypress/integration/embed/share_embed_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ describe("Embedded Queries", () => {
cy.getByTestId("QueryEditor")
.get(".ace_text-input")
.type("SELECT name, slug FROM organizations WHERE id='{{}{{}id}}'{esc}", { force: true });
// QueryEditor::onChange is debounced, so this wait is needed
cy.wait(500); // eslint-disable-line cypress/no-unnecessary-waiting

cy.getByTestId("TextParamInput").type("1");
cy.getByTestId("ParameterApplyButton").click();
Expand Down Expand Up @@ -68,8 +66,6 @@ describe("Embedded Queries", () => {
cy.getByTestId("QueryEditor")
.get(".ace_text-input")
.type("SELECT name, slug FROM organizations WHERE name='{{}{{}name}}'{esc}", { force: true });
// QueryEditor::onChange is debounced, so this wait is needed
cy.wait(500); // eslint-disable-line cypress/no-unnecessary-waiting

cy.getByTestId("TextParamInput").type("Redash");
cy.getByTestId("ParameterApplyButton").click();
Expand Down
2 changes: 0 additions & 2 deletions client/cypress/integration/query/create_query_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ describe("Create Query", () => {
cy.getByTestId("QueryEditor")
.get(".ace_text-input")
.type("SELECT id, name FROM organizations{esc}", { force: true });
// QueryEditor::onChange is debounced, so this wait is needed
cy.wait(500); // eslint-disable-line cypress/no-unnecessary-waiting

cy.getByTestId("ExecuteButton")
.should("be.enabled")
Expand Down

0 comments on commit 2934e53

Please sign in to comment.