Refactor useQueryExecution & add inline query execution status #4584

arikfr · 2020-01-23T10:47:32Z

What type of PR is this?

Refactor

Description

While trying to add an inline Query Execution status that won't remove the current results in display, I realized that we need to refactor useQueryExecution to allow the view to retain current results.

While the QueryResult class needs significant refactoring (probably split into two: one for execution and one for data), I did one change: add a callback to receive current execution status. This calllback allows us to remove the polling logic from useQueryResult and actually remove useQueryResult entirely. I did keep the getQueryResultData helper, but we should probably remove it when we get to do said refactoring.

I will add some more implementation notes in comments.

It's still not complete:

When triggering new execution while previous still running it messes up the status (both flashing).
Need better empty state for when no results are available (error or otherwise).
Empty state ("no results, try refresh") flashes on first load.
Make sure both View/Editor have the same query result information.
Add space between text and Cancel button.
Consider adding "inline" query execution status in Query Editor too.
Move QueryExecutionStatus elsewhere in mobile view.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

…k instead of polling on the query result status. This is a step towards bigger refacotr. We should completly rewrite this class and split it into two: 1. QueryExecution -- executes a query and tracks status. 2. QueryResult -- represents the actual QueryResult and has the different utilities to use the results (although those might be moved into helpers).

arikfr · 2020-01-23T10:48:30Z

client/app/pages/queries/QuerySource.jsx

@@ -154,20 +159,14 @@ function QuerySource(props) {
        return;
      }
      if (isDirty || !isEmpty(selectedText)) {
-        executeAdhocQuery(selectedText);
+        executeQuery(null, () => {


Need to improve the API here so we don't have to pass null as first argument if we're passing in custom executor.

arikfr · 2020-01-23T10:49:29Z

client/app/pages/queries/QueryView.jsx

  const [fullscreen, setFullscreen] = useState(false);
  const toggleFullscreen = useCallback(() => setFullscreen(!fullscreen), [fullscreen]);

-  useEffect(() => {
-    updateUrlSearch("fullscreen", fullscreen ? true : null);
-  }, [fullscreen]);


I removed the sync w/ URL on purpose. I'm not sure we need it here.

Yeah, I actually kept it thinking about sharing the URL and to have the same behavior the Dashboard page have. But both cases are not that important anyway.

arikfr · 2020-01-23T10:49:50Z

client/app/pages/queries/QueryView.jsx

-          {queryResult && queryResultData.status !== "done" && (
-            <div className="query-alerts m-t-15 m-b-15">
+          {(executionError || isExecuting) && (
+            <div style={{ position: "absolute", bottom: "75px", right: "20px", zIndex: 1000 }}>


Need to move this into the stylesheet.

arikfr · 2020-01-23T10:50:09Z

client/app/pages/queries/QueryView.jsx

              />
            </div>
          )}
-          {(!queryResult || queryResultData.status === "done") && (
+          {true && (


If we indeed keep viz always visible, need to remove this line.

I think this is probably the best option, the query view page just look so empty when loading and it makes the whole process of updating results look smoothier :)

arikfr · 2020-01-23T10:50:45Z

client/app/pages/queries/QueryView.jsx

                shortcut="alt+f"
                onClick={toggleFullscreen}>
-                <i className="zmdi zmdi-fullscreen" />
+                <Icon type={fullscreen ? "fullscreen-exit" : "fullscreen"} />


Switched to Ant's icons. If we like it, we should use it in other places where we have "fullscreen" toggle.

I'm leaned towards the zmdi one, but I think I'm biased anyway.

kravets-levko · 2020-01-23T11:07:18Z

client/app/pages/queries/hooks/useQueryExecute.js

+      newQueryResult
+        .toPromise(onStatusChange)
+        .then(queryResult => {
+          // TODO: this should probably belong in the QueryEditor page.


A quick note: here (and in other similar places) we need to handle a case when component was destroyed during async operation (ignore all .then/.catch and callback calls otherwise all sort of setStates will trigger error).

gabrieldutra · 2020-01-23T17:58:29Z

This is definitely in the direction I was thinking about (always having a result showing up). From the gif, imo the alert will need a different styling, I considered either having a notification (any from the Antd options) or just having the text saying the status of the execution in that footer. At most a colored Icon to emphasize the status.

gabrieldutra · 2020-02-04T00:26:01Z

client/app/pages/queries/hooks/useQueryExecute.js

-        query.latest_query_data_id = queryResult.getId();
-        query.queryResult = queryResult;
-      }
+    // TODO: this belongs on the query page?


I tend to prefer this logic in here mainly for 2 reasons: 1 - Reduce complexity from Pages; 2 - It's a shared piece of code between View and Source.

In terms of where this code "belongs", it feels to me it could be in both the query page or in here.

It's not for this PR, but eventually it makes sense to use useQueryExecute in Widgets, where this logic isn't needed.

gabrieldutra · 2020-02-04T00:28:10Z

client/app/pages/queries/hooks/useQueryExecute.js

+  useEffect(() => {
+    queryRef.current = query;
+    executeQueryRef.current = executeQuery;
+  }, [executeQuery, query]);


This is the safest way I could think to remove the dependencies from the useEffect hook and make it execute only once.

Is useEffect really needed here? Seems you can leave only assignment - assigning value to ref.current does not re-render component.

gabrieldutra · 2020-02-04T14:16:47Z

For "Empty state ("no results, try refresh") flashes on first load." I added a state to control whether the first results were loaded or not. So far I just keep it hidden until first results are loaded and it looks ok, apart from 2 cases:

If you use "maxAge" it will trigger a whole new query execution, thus take more time before showing up;
If you have a huge set of results. In this case I don't think introduce a loading state will solve the problem as the UI freezes for a while (I'm assuming it's due to the heavy transform calculations it does);

For 2, it's not in the scope in here, but explore WebWorkers for heavy calculations is passing through my mind (I think this could help with Dashboard performance issues as well).

gabrieldutra · 2020-02-05T00:59:32Z

client/app/pages/queries/hooks/useQueryExecute.js

+            }
+
+            setExecutionState({
+              queryResult,


There's also the option of keeping old queryResults when a query has an error, but you were thinking about an Error State?

🤔 not sure what I had in mind here. Will experiment with both options and decide.

gabrieldutra · 2020-02-05T18:20:54Z

@arikfr, I think most of the functionality side of this feature is done (I checked from the checkboxes in the description). Now it's mostly a matter of iterating design for both desktop and mobile.

For those I didn't understand very well what you mean:

"Need better empty state for when no results are available (error or otherwise)." -> (Refactor useQueryExecution & add inline query execution status #4584 (comment) should be related)
"Make sure both View/Editor have the same query result information." -> You mean to keep the query result information when you change between view/edit? (if it's what you meant, I can do it before iterating design)
"Consider adding "inline" query execution status in Query Editor too." -> Is it related to the "jumping" that happens on the existing position for the execution status?

gabrieldutra · 2020-02-11T20:01:40Z

No, I meant show the same information (and potentially reuse the same component). Right now we show different things in view and edit.

👍 I think I got it now, I updated the QueryExecutionMetadata to be a separate component. Now it's the same for both view and edit.

Updated 👍
I separated in two different cases and it's a matter of refining the message now. We'll also need to handle the case where the visualization should be rendered with 0 rows. I think the only existing case now is a Counter with countRows(?) If so, we can create an exception just for it now and open an issue to make it more general as it seems out of the scope here.
Added a min-width that covers the non-error messages 👍.
Updated 👍

arikfr · 2020-02-13T19:47:47Z

We'll also need to handle the case where the visualization should be rendered with 0 rows. I think the only existing case now is a Counter with countRows(?) If so, we can create an exception just for it now and open an issue to make it more general as it seems out of the scope here.

It makes sense for the visualization to decide how it handles 0 rows. Why not render the viz in such case?

arikfr · 2020-02-13T19:50:54Z

When clicking on "Add Description" it should auto focus the input field.

gabrieldutra · 2020-02-13T22:03:59Z

It makes sense for the visualization to decide how it handles 0 rows. Why not render the viz in such case?

🤔, the simplest should be to just pass an EmptyState node to the VisualizationRenderer. I'll see how easy it is to make the default behavior for visualizations to render this empty state, but also not be hard to replace it.

Edit: What I had in mind not to be changing all visualizations was to do sth similar to cff0163 (have a property that would define when a vis is going to use the provided emptyState)

- remove inputRef and use autoFocus attr

gabrieldutra · 2020-02-13T23:17:08Z

🪲 Just noticed there's some change specifically in this PR that breaks Filters (only multi-filters perhaps), seems to happening due to multiple React updates. E.g.: https://deploy-preview-4584--redash-preview.netlify.com/queries/334

Will investigate -> It seems sth to do with this (2f45fc5) Merge commit, I'll continue to see what may have gone wrong with it.

Fixed in cf56791. Looks like it was a missing useMemo that was triggering a loop update on a piece of code added from that merge commit 😬

arikfr · 2020-02-14T10:25:31Z

What will pass the empty state node? The viz implementation?

…

On Fri, Feb 14, 2020, 00:04 Gabriel Dutra ***@***.***> wrote: It makes sense for the visualization to decide how it handles 0 rows. Why not render the viz in such case? 🤔, the simplest should be to just pass an EmptyState node to the VisualizationRenderer. I'll see how easy it is to make the default behavior for visualizations, but also not be hard to replace it, — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4584?email_source=notifications&email_token=AAAROLASDAXG5UGBTGWBYGDRCW7VBA5CNFSM4KKULSI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELWZBGI#issuecomment-585994393>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAROLECNG2QJZTP5L5K57LRCW7VBANCNFSM4KKULSIQ> .

gabrieldutra · 2020-02-14T10:57:03Z

What will pass the empty state node? The viz implementation?

QueryPage pass emptyState -> General Vis Render checks if visualization isEmpty(data, options) (default to be rows.lenght == 0, but changeable by visualizations) and renders it if true. So by default what passes the emptyState is the context (query/widget), but it can be changed to the visualization by changing that isEmpty function. This is what I could think not to go on each visualization and add an empty state... Also as much as it makes sense for it to have each visualization controlling it's own empty state, it does for it to have a "Query" or "Widget" shared one... Anyway, lmk your opinion

arikfr · 2020-02-16T19:24:09Z

Let's minimize changes to the code or behavior and delegate to the visualization to handle this (which at the moment means do nothing -- like we had before).

I'll try to think about this by considering the various visualizations & states (query page vs. widget) and come up with a definition for future improvement.

arikfr · 2020-02-19T13:37:11Z

@gabrieldutra I did two small changes here:

Add label to the field Data Source field.
Hide navbar in full screen mode (and reuse dashboard's useFullscreenHandler).

There are still few small design fixes to do, but I will merge this (and the parent PR) now and open a separate issue detailing them.

arikfr added 2 commits January 23, 2020 10:52

Refactor useQueryExecution.

8e52b28

arikfr added Frontend UX labels Jan 23, 2020

arikfr requested review from gabrieldutra and kravets-levko January 23, 2020 10:47

arikfr commented Jan 23, 2020

View reviewed changes

kravets-levko reviewed Jan 23, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Jan 27, 2020

Weekly Digest (20 January, 2020 - 27 January, 2020) #4591

Closed

gabrieldutra added 3 commits February 3, 2020 14:16

useQueryExecution: only consider last triggered execution

52ebb76

Fix execution triggered on every query update

8d8b1c3

Add loadedInitialResults and fix loading results issues

63c34f4

gabrieldutra reviewed Feb 4, 2020

View reviewed changes

gabrieldutra added 3 commits February 3, 2020 22:37

Only show Add Vis button when results are done

d16494b

Merge branch 'query-view' into refactor-execution

2f45fc5

Don't show notification on first results load

c8b916c

gabrieldutra added 3 commits February 4, 2020 14:10

Remove unnecessary useEffect

3ae2f74

Add margin between text and cancel button

5364594

Add Refresh button to QuerySource

e7ca220

gabrieldutra reviewed Feb 5, 2020

View reviewed changes

gabrieldutra added 4 commits February 6, 2020 15:19

Update ExecutionStatus positioning

288f84d

Mobile updates

2e7bf11

Add more spacing for execution status in mobile

bf352c8

Merge branch 'query-view' into refactor-execution

d1ef80a

gabrieldutra force-pushed the refactor-execution branch from 15d0e36 to d1ef80a Compare February 9, 2020 14:54

weekly-digest bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #4633

Closed

gabrieldutra added 6 commits February 11, 2020 14:27

Hide Add Description/tag on mobile

0f4d66f

Allow different messages for no results/empty set

1fd396b

Don't show empty state when opening new query page

278de51

Add min-width to ExecutionStatus

c6312b3

Improve ExecutionStatus spacing

ed37620

Use same execution information for view/edit

f0338ce

gabrieldutra force-pushed the refactor-execution branch from 3db2377 to f0338ce Compare February 11, 2020 19:54

Use autoFocus in EditInPlace for focus

3c981c7

- remove inputRef and use autoFocus attr

gabrieldutra added 2 commits February 13, 2020 20:26

Control visualization row emptyState from Renderer

cff0163

Fix broken Filters on Visualization Renderer

cf56791

weekly-digest bot mentioned this pull request Feb 17, 2020

Weekly Digest (10 February, 2020 - 17 February, 2020) #4652

Closed

Don't handle empty state based on context

50710b2

arikfr added this to the v9 milestone Feb 18, 2020

arikfr marked this pull request as ready for review February 19, 2020 10:09

arikfr added 2 commits February 19, 2020 15:30

Add Data Source label to data source.

4041f83

Reuse useFullscreenHandler in QueryView page.

e526173

restyled-io bot mentioned this pull request Feb 19, 2020

Restyle Refactor useQueryExecution & add inline query execution status #4662

Closed

(style fix)

75820d8

arikfr merged commit c6936a1 into query-view Feb 19, 2020

arikfr deleted the refactor-execution branch February 19, 2020 13:37

weekly-digest bot mentioned this pull request Feb 24, 2020

Weekly Digest (17 February, 2020 - 24 February, 2020) #4678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor useQueryExecution & add inline query execution status #4584

Refactor useQueryExecution & add inline query execution status #4584

arikfr commented Jan 23, 2020 •

edited by gabrieldutra

Loading

arikfr Jan 23, 2020

arikfr Jan 23, 2020

gabrieldutra Jan 23, 2020

arikfr Jan 23, 2020

arikfr Jan 23, 2020

gabrieldutra Jan 23, 2020

arikfr Jan 23, 2020

gabrieldutra Jan 23, 2020

kravets-levko Jan 23, 2020

arikfr Jan 23, 2020

gabrieldutra commented Jan 23, 2020

gabrieldutra Feb 4, 2020

arikfr Feb 10, 2020

gabrieldutra Feb 4, 2020 •

edited

Loading

kravets-levko Feb 4, 2020

gabrieldutra commented Feb 4, 2020

gabrieldutra Feb 5, 2020

arikfr Feb 10, 2020

gabrieldutra commented Feb 5, 2020

gabrieldutra commented Feb 11, 2020

arikfr commented Feb 13, 2020

arikfr commented Feb 13, 2020

gabrieldutra commented Feb 13, 2020 •

edited

Loading

gabrieldutra commented Feb 13, 2020 •

edited

Loading

arikfr commented Feb 14, 2020 via email

gabrieldutra commented Feb 14, 2020 •

edited

Loading

arikfr commented Feb 16, 2020

arikfr commented Feb 19, 2020

Refactor useQueryExecution & add inline query execution status #4584

Refactor useQueryExecution & add inline query execution status #4584

Conversation

arikfr commented Jan 23, 2020 • edited by gabrieldutra Loading

What type of PR is this?

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Feb 5, 2020

gabrieldutra commented Feb 11, 2020

arikfr commented Feb 13, 2020

arikfr commented Feb 13, 2020

gabrieldutra commented Feb 13, 2020 • edited Loading

gabrieldutra commented Feb 13, 2020 • edited Loading

arikfr commented Feb 14, 2020 via email

gabrieldutra commented Feb 14, 2020 • edited Loading

arikfr commented Feb 16, 2020

arikfr commented Feb 19, 2020

arikfr commented Jan 23, 2020 •

edited by gabrieldutra

Loading

gabrieldutra Feb 4, 2020 •

edited

Loading

gabrieldutra commented Feb 13, 2020 •

edited

Loading

gabrieldutra commented Feb 13, 2020 •

edited

Loading

gabrieldutra commented Feb 14, 2020 •

edited

Loading