feat: Pass dashboard context to explore through local storage #20743

kgabryje · 2022-07-18T13:55:19Z

SUMMARY

Currently when user clicks "Edit chart" or chart's title in Dashboard page, we first send a POST request to retrieve form_data_key, then create an Explore URL with that form_data_key and redirect to Explore. We do that in order to pass dashboard context (such as native filters, cross filter, color scheme...) to Explore.
There are several problems with this approach:

we redirect to another page, so from semantic HTML point of view this element should be a link
user can't access standard link contextual menu when right clicking
there's a noticeable delay between a click and redirection because of HTTP request being made in the middle
we unnecessarily take up space in redis

In order to refactor "Edit chart" to be a link, we need to pass dashboard context in a different way.

This PR implements the following changes:

save current dashboard state in local storage, where random id generated on Dashboard mount is a key and stringified state is a value
update the value in local storage on each dashboard state change (such as applying new native filter or cross filter or changing color scheme)
"Edit chart"/chart title is a React Router Link and it contains local storage key as a search param
when mounting Explore, retrieve the dashboard state from local storage using the dashboard id search param and merge it with form data coming from initializing Explore request

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-07-18.at.15.46.56.mov

TESTING INSTRUCTIONS

Open dashboard
Apply native filters, cross filters, filter box filters
Open Explore by clicking on chart title or clicking "Edit chart". Try opening in the same tab and opening in a new tab
Verify that filters from dashboard are present in Explore

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

kgabryje · 2022-07-18T20:53:19Z

/testenv up

github-actions · 2022-07-18T20:55:09Z

@kgabryje Ephemeral environment spinning up at http://54.203.155.41:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

codecov · 2022-07-19T09:14:40Z

Codecov Report

Merging #20743 (0ea75cf) into master (644148b) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #20743      +/-   ##
==========================================
- Coverage   66.33%   66.29%   -0.04%     
==========================================
  Files        1756     1757       +1     
  Lines       66769    66867      +98     
  Branches     7059     7077      +18     
==========================================
+ Hits        44288    44332      +44     
- Misses      20684    20726      +42     
- Partials     1797     1809      +12

Flag	Coverage Δ
javascript	`51.95% <50.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ages/superset-ui-core/src/query/types/Dashboard.ts	`100.00% <ø> (ø)`
superset-frontend/src/dashboard/actions/hydrate.js	`1.96% <0.00%> (-0.11%)`	⬇️
...src/dashboard/components/FiltersBadge/selectors.ts	`70.37% <0.00%> (+1.90%)`	⬆️
...dashboard/components/SliceHeaderControls/index.tsx	`65.88% <0.00%> (+2.35%)`	⬆️
...board/components/nativeFilters/FilterBar/index.tsx	`60.14% <0.00%> (-0.44%)`	⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts	`46.66% <ø> (ø)`
...perset-frontend/src/dashboard/containers/Chart.jsx	`75.00% <ø> (ø)`
...set-frontend/src/dashboard/containers/Dashboard.ts	`0.00% <ø> (ø)`
...shboard/util/charts/getFormDataWithExtraFilters.ts	`88.88% <ø> (-5.56%)`	⬇️
superset-frontend/src/explore/ExplorePage.tsx	`0.00% <0.00%> (ø)`
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

kgabryje · 2022-07-19T17:43:44Z

@jinghua-qa Can you help with testing please?

kgabryje · 2022-07-19T17:56:33Z

/testenv up

kgabryje · 2022-07-19T17:58:19Z

superset-frontend/src/dashboard/actions/hydrate.js

+        element => element.id !== DASHBOARD_ROOT_ID && !element.parents,
+      )
+    ) {
+      updateComponentParentsList({


Some sample dashboards don't have parents in their layout metadata, which caused e2e tests to fail

Can we fix the sample dashboards metadata instead of introducing this logic? We can do it in a follow-up if needed.

kgabryje · 2022-07-19T17:59:59Z

superset-frontend/src/dashboard/actions/hydrate.js

+          behaviors.includes(Behavior.INTERACTIVE_CHART) &&
+          !metadata.chart_configuration[chartId].crossFilters?.chartsInScope
+        ) {
+          metadata.chart_configuration[chartId].crossFilters.chartsInScope =


Optimization for cross filters (works the same as the one implemented for native filters a few months ago) to speed up finding charts in scope. Also, thanks to that, we don't need to save dashboard layout in local storage

github-actions · 2022-07-19T18:00:53Z

@kgabryje Ephemeral environment spinning up at http://54.245.54.96:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2022-07-19T18:04:01Z

superset-frontend/src/dashboard/containers/DashboardPage.tsx

+  );
+  return Object.fromEntries(
+    Object.entries(dashboardsContexts).filter(
+      ([, value]) => !value.isRedundant,


A new dashboard tab id is generated on each dashboard page opening. We mark ids as redundant when user leaves the dashboard, because they won't be reused. Then we remove redundant dashboard contexts from local storage in order not to clutter it

kgabryje · 2022-07-19T18:20:02Z

superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts

+  return filterBoxData;
+};
+
+const mergeNativeFiltersToFormData = (


This logic is copied from /superset/utils/core.py::merge_extra_form_data

kgabryje · 2022-07-19T18:21:21Z

superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts

+  }, [] as SimpleAdhocFilter[]);
+};
+
+const mergeFilterBoxToFormData = (


This logic is copied from /superset/utils/core.py::merge_extra_filters

ktmud

Thanks for working on this! LGTM in general, just a couple of questions.

ktmud · 2022-07-18T16:59:36Z

superset-frontend/src/dashboard/containers/DashboardPage.tsx

+  );
+  const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+    getActiveFilters(),
+  );


Can we consolidate all these into one single state object, and maybe reuse DashboardPermalinkState (or at least extend from it and consolidate variable names) for it? We can rename it to something like PersistableDashboardState if necessary.

I ended up creating a new state interface. The state that we save in local storage is trimmed specifically for the use of Explore in order to reduce the size of saved payload (to reduce the risk of overflowing the local storage), so extending interfaces used in Dashboard wasn't applicable

ktmud · 2022-07-18T17:04:01Z

superset-frontend/src/explore/ExplorePage.tsx

+      sharedLabelColors,
+      colorScheme,
+      chartConfiguration,
+      nativeFilters,


Is nativeFilters the full metadata of all native filters? Do we really need to carry it over to the Explore page? I don't see it being used here.

It's state.nativeFilters.filters (so without filterSets). We use it in getFormDataWithExtraFilters called in line 88. However, now I noticed that we only need chartsInScope for each filter - the rest of metadata is redundant. Same goes for cross filters (chartConfiguration). I'll change that

ktmud · 2022-07-18T17:18:02Z

superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts

+  return newFormData;
+};
+
+export const getFormDataFromDashboardContext = (formData: JsonObject) => {


I like this util function!

ktmud · 2022-07-18T17:27:46Z

superset-frontend/src/dashboard/components/SliceHeader/index.tsx

@@ -97,6 +98,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
 }) => {
  const dispatch = useDispatch();
  const uiConfig = useUiConfig();
+  const dashboardTabId = useContext(DashboardTabIdContext);


Why do we use a new ReactContext for this? Can this be a Redux state?

We only need to pass dashboardTabId deep into the component tree, with no intention of ever modifying it or interacting with other redux state variables. I think for such use cases Context is optimal

This all good. I'm not a fan of Redux anyway... just thought creating a new provider has its own overhead---i.e., too many nested context provider always feels wrong.

ktmud · 2022-07-18T17:29:26Z

superset-frontend/src/dashboard/containers/DashboardPage.tsx

+  const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+    getActiveFilters(),
+  );
+  const dashboardTabId = useMemo(() => shortid.generate(), []);


Can we rename this to dashboardPageId to avoid confusion with dashboard tabs?

Good point!

ktmud · 2022-07-18T17:32:34Z

superset-frontend/src/dashboard/containers/DashboardPage.tsx

+      updateDashboardTabLocalStorage(dashboardTabId, {
+        ...payload,
+        isRedundant: true,
+      });


Instead of adding a isRedundant flag, can we just remove it?

That might lead to race conditions between removing local storage key and consuming it in Explore.
Case 1: You open Explore in a new tab and close the tab with dashboard before Explore mounts - key doesn't exist anymore in Explore.
Case 2: You open Explore in the same tab as dashboard - Dashboard component unmounts and removes the key before Explore mounts. That case could be handled by always removing the key in Explore instead of on Dashboard unmount. However, that leads us to Case 3: you open Explore in a new tab, key is removed on Explore mount, then you go back to the tab with dashboard and open a new tab with Explore - key doesn't exist anymore in this tab.

I think flagging key as redundant and removing it on next dashboard action is the safest approach here

This makes sense. Thanks for the explanation.

ktmud · 2022-07-18T22:55:27Z

superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx

-              {t('Edit chart')}
-            </Tooltip>
+          <Menu.Item key={MENU_KEYS.EXPLORE_CHART}>
+            <Link to={this.props.exploreUrl ?? '#'}>


Instead of ?? '#' why not just remove the whole link in case url is not available?

Oops, forgot to remove that. Now exploreUrl is always defined so we don't need a null check.

ktmud · 2022-07-18T22:58:47Z

superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx

+                <Button buttonStyle="secondary" buttonSize="small">
+                  <Link to={this.props.exploreUrl ?? '#'}>
+                    {t('Edit chart')}
+                  </Link>


Would <Link > actually add an <a> inside a button element?

Should we use react-router-dom's useHistory instead?
https://v5.reactrouter.com/web/api/Hooks/usehistory

Yeah it does. I'll change that to onClick with history.push

ktmud · 2022-07-19T18:16:51Z

superset-frontend/src/explore/ExplorePage.tsx

+      filters: Object.fromEntries(
+        (
+          Object.entries(filterBoxFilters) as [
+            string,
+            {
+              scope: number[];
+              values: DataRecordValue[];
+            },
+          ][]
+        )
+          .filter(([, filter]) => filter.scope.includes(sliceId))
+          .map(([key, filter]) => [
+            getChartIdAndColumnFromFilterKey(key).column,
+            filter.values,
+          ]),


Can L90 to L104 be of its own utility function?

michael-s-molina · 2022-07-20T14:47:46Z

Thanks for the PR @kgabryje. This is a nice improvement now that we have the permalink feature in place and form data keys are not shared anymore.

I noticed that the form_data_key is still being generated in Explore. Is there a reason for it? If it's not needed, can we remove it and mark the explore/form_data endpoint as deprecated? You can add a deprecation warning to the API file.

kgabryje · 2022-07-20T19:01:40Z

I noticed that the form_data_key is still being generated in Explore. Is there a reason for it? If it's not needed, can we remove it and mark the explore/form_data endpoint as deprecated? You can add a deprecation warning to the API file.

The logic related to form_data_key is still used in a lot of places in Explore code. We should handle that separately, since this PR focuses on refactoring linking from Dashboard to Explore.

michael-s-molina

Code LGTM. Can we get @jinghua-qa's approval before merging the PR? @jinghua-qa It would be a good idea to run our complete test suite on this to make sure everything is working 😉

michael-s-molina · 2022-07-21T11:52:39Z

superset-frontend/src/dashboard/actions/hydrate.js

+        element => element.id !== DASHBOARD_ROOT_ID && !element.parents,
+      )
+    ) {
+      updateComponentParentsList({


Can we fix the sample dashboards metadata instead of introducing this logic? We can do it in a follow-up if needed.

jinghua-qa · 2022-07-21T15:45:38Z

Code LGTM. Can we get @jinghua-qa's approval before merging the PR? @jinghua-qa It would be a good idea to run our complete test suite on this to make sure everything is working 😉

OK, i can create a test branch and deploy a workspace to run automation today.

jinghua-qa · 2022-07-22T07:07:37Z

/testenv up

github-actions · 2022-07-22T10:31:33Z

@jinghua-qa Ephemeral environment spinning up at http://52.24.5.129:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-07-24T08:14:38Z

/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true

github-actions · 2022-07-24T08:18:59Z

@jinghua-qa Ephemeral environment spinning up at http://35.164.111.177:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-07-25T05:49:25Z

I found one issue that when open the chart from dashboard with color theme, and then save the chart as new chart, the color theme for the new chart still not able to change the color theme. Other than that i think the changes LGTM.

Screen.Recording.2022-07-24.at.1.47.14.AM.mov

kgabryje · 2022-07-25T12:54:38Z

Thank you for spotting that @jinghua-qa ! Fixed

Screen.Recording.2022-07-25.at.14.52.18.mov

github-actions · 2022-07-25T17:05:52Z

Ephemeral environment shutdown and build artifacts deleted.

openaphid · 2022-07-27T15:18:04Z

I'm afraid it breaks DASHBOARD_EDIT_CHART_IN_NEW_TAB: True. Is there a plan to remove this feature flag?

kgabryje · 2022-07-27T15:33:53Z

I'm afraid it breaks DASHBOARD_EDIT_CHART_IN_NEW_TAB: True. Is there a plan to remove this feature flag?

Yes you're right, that flag is no longer needed. Now you can click "Edit chart" like any other link to open in a new tab - either cmd + click, or right click + "open in a new tab", or middle mouse button.
I'll remove that feature flag as a follow up

john-bodley · 2022-09-09T15:53:06Z

superset-frontend/src/explore/ExplorePage.tsx

+      filterBoxFilters,
+      dataMask,
+      dashboardId,
+    } =


@kgabryje I believe this logic introduced a regression, i.e., dataMask could be undefined resulting in an error being thrown here. Would you be able to look into this issue?

kgabryje requested review from ktmud, zhaoyongjie, geido and michael-s-molina July 18, 2022 13:55

pull-request-size bot added the size/XL label Jul 18, 2022

pull-request-size bot added size/XXL and removed size/XL labels Jul 19, 2022

kgabryje commented Jul 19, 2022

View reviewed changes

ktmud reviewed Jul 19, 2022

View reviewed changes

kgabryje force-pushed the feat/dashboard-to-explore-link branch from 39644b5 to cc8ea6f Compare July 20, 2022 21:06

michael-s-molina approved these changes Jul 21, 2022

View reviewed changes

feat: Pass dashboard context to explore through local storage

412f184

kgabryje added 15 commits July 25, 2022 14:37

Remove console log

066e477

Remove unused local storage keys

7ab049b

Fix lint

645c396

Fix link

2a08622

Fix UT

63b790f

fix lint

9369185

fix prettier

7dc2c10

Fix bug

fd105dc

Fix bug with some sample dashboards

1aa1ed6

Roll back unnecessary change

c3a721e

style fix

d326d64

Add comments

87cde77

Fix lint

58d2bd0

Address code review comments

f3b2027

Fix

0ea75cf

kgabryje force-pushed the feat/dashboard-to-explore-link branch from cc8ea6f to 0ea75cf Compare July 25, 2022 12:53

kgabryje merged commit 0945d4a into apache:master Jul 25, 2022

kgabryje mentioned this pull request Jul 31, 2022

fix(dashboard): Incorrect scopes of cross filters #20927

Merged

9 tasks

AAfghahi added the logging Creates a UI or API endpoint that could benefit from logging. label Aug 5, 2022

rusackas mentioned this pull request Aug 9, 2022

fix: making crossFilter.chartsInScope optional #21030

Merged

9 tasks

john-bodley reviewed Sep 9, 2022

View reviewed changes

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

feat: Pass dashboard context to explore through local storage #20743

feat: Pass dashboard context to explore through local storage #20743

Conversation

kgabryje commented Jul 18, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

kgabryje commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

kgabryje commented Jul 19, 2022

kgabryje commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 19, 2022

Choose a reason for hiding this comment

kgabryje Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

kgabryje Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Jul 18, 2022 • edited Loading

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

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

michael-s-molina commented Jul 20, 2022

kgabryje commented Jul 20, 2022

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinghua-qa commented Jul 21, 2022

jinghua-qa commented Jul 22, 2022

github-actions bot commented Jul 22, 2022

jinghua-qa commented Jul 24, 2022

github-actions bot commented Jul 24, 2022

jinghua-qa commented Jul 25, 2022

kgabryje commented Jul 25, 2022

github-actions bot commented Jul 25, 2022

openaphid commented Jul 27, 2022

kgabryje commented Jul 27, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2022 •

edited

Loading

kgabryje Jul 19, 2022 •

edited

Loading

kgabryje Jul 19, 2022 •

edited

Loading

kgabryje Jul 19, 2022 •

edited

Loading

ktmud Jul 18, 2022 •

edited

Loading

ktmud Jul 18, 2022 •

edited

Loading