[dashboard] After update filter, trigger new queries when charts are visible

[graceguo-supercat]

SUMMARY

Currently Superset dashboard only support global filters, which means filter will apply to all charts in the dashboard (except the immune ones).

After user apply filter, dashboard will trigger new queries for all charts in DOM, even for those under other tabs. Many users feel confused whether charts under other tabs are affected by filter, or which charts are affected by filter. At the same time, if there are many charts in DOM, trigger queries for charts under other invisible tabs will make charts in current tab looks very slow.

Proposed solution:
After user updates a filter, trigger new queries when charts are visible.

• If both filter and chart are visible (under same tab), new queries should be triggered right away.
• If charts are not visible when filter is updated, chart will not be updated at the same time. But when user click into a new tab, all the charts become visible should trigger updated queries.

Implementation and Performance:

• Dashboard used react-bootstrap TabContainer as root level tab. To avoid expensive mounts/unmounts of the entire dashboard upon adding/removing top-level tabs, we set unmountOnExit={false}. This means it will not remove rendered charts from the DOM when they are no longer visible. And when charts data/state is updated, each chart will re-render itself.

• old filter flow:
  User update a filter,
  => change filter state in redux
  => Dashboard component pass down the changes to its children components
  => in Dashboard componentDidUpdate: call runQuery async action creator for every chart, which will trigger a new query for every chart
  => dashboard/components/gridComponents/Chart.jsx decide shouldComponentUpdate: only check if chart has valid props changes.
  => after chart's state is updated, each chart will trigger its render function

When dashboard calling runQuery for each chart, it doesn't check visibility of the charts. Since tab/charts visibility is stored in component state not global redux, dashboard component has no awareness of each chart visibility. It will send updated queries even when charts are not visible. This update all charts together solution has another lack for clearance: when user clicks another tab, the data is already updated, but there is no clear indicator to tell user when these charts are updated.

• new filter flow:
  Instead of using Dashboard component to trigger query for each chart, new filter flow delegate the logic to component (react component props and redux state).
  • Chart component: added componentDidUpdate. After any chart is mounted, if its query parameters are changed (from props), we should allow chart run updated query and re-render.
  • Components in dashboard (row, column, tab, tabs, chartHolder): add isComponentVisible props. It passes visibility status from parents to it children component.

User update a filter,
=> change filter state in redux
=> Dashboard component pass down the changes to its children components
=> in Dashboard component componentDidUpdate: set each chart's triggerQuery flag be true, which mean chart should be updated. Here we don't trigger query anymore.
=> dashboard/components/gridComponents/Chart.jsx decide shouldComponentUpdate: besides valid props changes, chart only updates when it's visible.
=> chart's props changes. In componentDidUpdate, if there were a filter update, chart will have triggerQuery flag set true. And chart's query parameters are also updated, it will trigger a new query and re-render after data come back.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

1. create dashboard with root level tabs and row level tabs. there should have a filter_box in dashboard.
2. load dashboard, visit charts in different tabs.
3. Change filter box value, you should see queries from charts in the same tab were sent to server-side.
4. select another tab, you will see queries from current visible tabs were sent out.

ADDITIONAL INFORMATION

[ ] Has associated issue:
[ ] Changes UI
[ ] Requires DB Migration. Confirm DB Migration upgrade and downgrade tested.
[ ] Introduces new feature or API
[ ] Removes existing feature or API
[x] Fixes bug
[x] Refactors code
[x] Adds test(s)

REVIEWERS

@michellethomas @kristw @xtinec @williaster @mistercrunch

[codecov-io]

Codecov Report

Merging #7233 into master will increase coverage by 0.02%.
The diff coverage is 35.08%.

@@            Coverage Diff             @@
##           master    #7233      +/-   ##
==========================================
+ Coverage   65.08%   65.11%   +0.02%     
==========================================
  Files         429      430       +1     
  Lines       21002    21044      +42     
  Branches     2338     2342       +4     
==========================================
+ Hits        13669    13702      +33     
- Misses       7217     7226       +9     
  Partials      116      116

Impacted Files	Coverage Δ
...ts/src/dashboard/components/gridComponents/Row.jsx	87.5% <ø> (ø)	⬆️
...s/src/dashboard/components/gridComponents/Tabs.jsx	63.93% <ø> (ø)	⬆️
...ashboard/components/gridComponents/ChartHolder.jsx	84.84% <ø> (ø)	⬆️
...ts/src/dashboard/containers/DashboardComponent.jsx	100% <ø> (ø)	⬆️
...ts/src/explore/components/ExploreViewContainer.jsx	45.71% <ø> (+13.79%)	⬆️
...sets/src/dashboard/components/DashboardBuilder.jsx	81.81% <ø> (ø)	⬆️
...src/dashboard/components/gridComponents/Column.jsx	87.87% <ø> (ø)	⬆️
.../assets/src/dashboard/components/DashboardGrid.jsx	61.11% <ø> (ø)	⬆️
...rset/assets/src/dashboard/containers/Dashboard.jsx	0% <ø> (ø)	⬆️
superset/assets/src/chart/Chart.jsx	10.86% <0%> (-1.64%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46579b1...cb86b53. Read the comment docs.

[williaster]

LGTM overall, had one question and wanted to confirm chart updates happen as expected in explore view?

[williaster]

should this be "row" => "top"?

[graceguo-supercat]

in integration test for dashboard, i created layout a like this (so that i can test current solution works for both row-level tab and root-level tab):

root level tab A: has only 1 row

• chart-1

root level tab B: has 2 rows

• row 1: chart-2
• row 2: row-level tabs
  • row-level tab 1: filter
  • row-level tab 2: chart-3

when i update filter, chart-2 is always visible, so it get updated right away.
when i click row-level tab 2, i can see chart-3 become visible, and it sends new query.
when i click root-level tab A, i can see chart-1 becomes visible, and it sends new query.

[williaster]

ditto "row"?

[williaster]

sorry not sure I quite understand the explore view changes with the new flow, why do we remove this line? I get that src/chart/Chart will now trigger a query if its prop.formData prop changes, but its props.triggerQuery still has to be true right? does that only happen with this line or something else causes that?

[graceguo-supercat]

When user change viz_type (or any control that needs trigger new query), we should show a overlay to hold the change, and allow user to update more controls. when user clicks on Run query we trigger new query with all change together. But in current master branch code, change viz_type will trigger new query right away (but not update UI because of overlay):

This line of change (not setting triggerQuery) is just to fix this additional query issue.

[williaster]

LGTM!