[Dashboard] Rebuild State Management #97941

ThomThomson · 2021-04-22T00:14:07Z

Summary

This rather large PR rebuilds and reorganizes the dashboard state management system. Most prominently, this PR finally removes all app state from the dashboard URL. This effort is also aimed at creating a better architecture and developer experience within the dashboard app to make debugging easier, provide a stable foundation for major upcoming features.

Fixes #21701
Fixes #92085
Fixes #98087
Fixes #99190
Addresses 2/3 line items from #91950

Architecture Diagram

Overall, removing URL syncing greatly simplified the architecture. The following diagram shows the entirety of dashboard state management.

User Facing Changes

Full Unsaved State Persistence

The full state of your dashboard is now persisted in session storage, including options, query, filters etc. Previously, only the panel state was backed up there, causing any other unsaved state change to be lost when navigating away from the dashboard.

Click for Gifs

Before- options were not saved in the session storage. When leaving the dashboard and re-entering the options are reset to their defaults.

After- options are saved to the session store along with all other pieces of dashboard state.

No more View Mode with Unsaved Changes

Now that the app state has been fully removed from the URL. the browser back / forward buttons should only work for navigation. This means that it is no longer possible to exit edit mode without a warning by pressing the back button. Because of this, we are again able to prevent the user from leaving edit mode when they have unsaved changes. This is a much simpler UX, and the unsaved changes warning from before 7.12 has been restored.

Click for images

Before- in 7.12 a third option was added to the switch to view mode dialog which would allow users to switch back to view mode while keeping their changes. This was necessary because of the URL state, but led to confusing and complicated UX.

After- We have restored the pre 7.12 modal, with only two options:

Better change checking for Panels

This PR changes dashboard panel state to a map instead of an array. This, combined with a better diffing method, means that the order of panels no longer matters in the diff. This cleans up a few inconsistencies with the unsaved changes message appearing when it shouldn't.

Click for Gifs

Before- Notice how even though the panels are in the exact same orientation that they were in before, the unsaved changes badge is still present. This particular order of moving panels 'reorders' them in the array, causing a diff.

After- The same steps were taken in the new version, but the diff only takes into account the positions of each panel. See how the unsaved changes badge disappears here.

Better change checking for Options

This PR changes the change checking for options to allow for undefined or null values. Optional values would previously trip up the diff and cause unsaved changes to appear when it shouldn't.

Click for Gifs

Before- Even when the option was set back to its last saved state, the unsaved changes badge was still present

After- This issue has been fixed.

Stay in Edit mode on save as

Saving a dashboard will now never boot you out of edit mode. Quick save used to be the only method that did this, which was inconsistent.

Click for Gif

Silent Panel Migrations

Ever since 7.3, the versions that Panels were last saved under caused unnecessary diffing in the dashboard panels. This meant that any older version would always show unsaved changes even if a migration was unnecessary.

Now, because panels are compared as a map of panel states the version numbers will not get in the way and unsaved changes badge should only show due to direct user action.

Developer Experience Changes

One medium sized useEffect hook now handles all setup tasks
Redux Toolkit is now used to store dashboard state in memory
The code is now more functional, with similar functions grouped into files under /lib
Redux state is now the only source of truth. State is copied to the Dashboard Container and the Session storage
All dashboard state is treated the same. It is no longer half persisted in session storage and half persisted in the URL.
Change checking is done in one place, and is debounced to increase performance

Known Issues

The silent panel migrations feature listed above does not work with maps panels. If you load an older dashboard which has a map, (including any demo data dashboards) the unsaved changes badge will appear. This is due to an unrelated issue #98180

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

For maintainers

This was checked for breaking API changes and was labeled appropriately

…panel map for better Diffing. Discard Unsaved Changes

…te, cleaned up jest tests

…oardRedux

…sync getting old state

…oardRedux

… multiple history records when removing state from URL

…oardRedux

flash1293 · 2021-05-05T09:51:53Z

It looks like this PR is changing the "sync color" mode for already saved dashboards. What do you think about treating all dashboards which are saved already as "sync color" enabled, but the flag will be disabled for newly created dashboards? This way we don't change the behavior for saved objects the user has already.

nreese

maps changes LGTM
code review

…oardRedux

ThomThomson · 2021-05-19T18:16:57Z

@elasticmachine merge upstream

ThomThomson · 2021-05-25T14:25:23Z

@elasticmachine merge upstream

clintandrewhall

@ThomThomson, this is an incredibly impressive amount of work... I know it feels good to get this kind of tech debt off the table! It's really exciting.

I'm going to approve this PR, but I have to say, it's only from a literal and style perspective. It's really, really difficult to review the logic because of the sheer number of lines, but also the density line-by-line.

I'm erring on the side of getting it in so we can shake it out, rather than delaying to break it down further... but it makes me uncomfortable when I err on that side too often.

That said, what makes me confident in this PR is your obvious attention to detail and extensive tests. I wish all PRs I've reviewed had these!! ❤️

I know we've already discussed how this PR likely couldn't be split up, and that's fine. I think if the code were structured a bit differently, as I've suggested above, it would be easier for folks who haven't written it to understand the scope of the changes. Code comments help here, as well.

Let's get this important change in, but let's also talk more about how we can make Dashboard code smaller, more portable, grepable, and thus more "reviewable".

Great work, Devon!

clintandrewhall · 2021-05-26T13:45:15Z

src/plugins/dashboard/public/application/lib/dashboard_session_restoration.ts

+    dashboardId: getDashboardId(),
+    timeRange: shouldRestoreSearchSession
+      ? data.query.timefilter.timefilter.getAbsoluteTime()
+      : data.query.timefilter.timefilter.getTime(),


This is a common fixture in the code in this PR: passing in large objects and restating the chain. It makes the code harder to read, but also hard to review.

The current practice also means useEffect calls monitor the entire object for changes, rather than destructured portions drawn from props in the start of the render cycle. So it's a good habit to adopt in general.

Consider:

const { filterManager, queryString } = data.query; const { timefilter } = data.query.timefilter; // ... timeRange: shouldRestoreSearchSession ? timefilter.getAbsoluteTime() : timefilter.getTime(), // ... filters: filterManager.getFilters, query: queryString.formatQuery(appState.query), // ...etc

This particular example doesn't affect the useEffect, but I've reorganized it as suggested to be more clean.

clintandrewhall · 2021-05-26T13:49:27Z

src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts

+  DashboardState,
+} from '../../types';
+
+interface DashboardDiffCommon {


nit: type DashboardDiffCommon = Record<string, unknown>;

I've seen this written both ways. Is there any reason to use one over the other?

clintandrewhall · 2021-05-26T13:50:17Z

src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts

+  newInput: DashboardContainerInput
+) => {
+  return commonDiffFilters<DashboardContainerInput>(
+    (originalInput as unknown) as DashboardDiffCommonFilters,


This kind of cast usually indicates a "code smell". It's hard to know in this case, though.

There are a lot of casts in this PR, which is a bit concerning.

This is a result of me perhaps over-DRY-ing this section. I'm casting this just within this file in order to apply the same function (diffing the filters) to two different kinds of dashboard state.

The main thing that I'm proud of here is treating the diffing of both types of dashboard state the same, and diffing them in the same file. In the past this was done in two totally different ways, in different files. In the future, I'd like to simplify this even further by combining DashboardState and DashboardContainerInput into one type.

clintandrewhall · 2021-05-26T13:52:02Z

src/plugins/dashboard/public/application/lib/save_dashboard.ts

+  currentState: DashboardState;
+  timefilter: TimefilterContract;
+  saveOptions: SavedDashboardSaveOpts;
+  toasts: NotificationsStart['toasts'];


This just reminds me we need to get our service abstractions into Dashboard. :-)

Definitely! That is the next tech-debt item to tackle in dashboard for sure!

clintandrewhall · 2021-05-26T13:55:03Z

src/plugins/dashboard/public/application/lib/save_dashboard.ts

+  savedDashboard.title = currentState.title;
+  savedDashboard.description = currentState.description;
+  savedDashboard.timeRestore = currentState.timeRestore;
+  savedDashboard.optionsJSON = JSON.stringify(currentState.options);


nit: you might consider the following to reduce visual repetition/noise. It also makes what you're using and what you're replacing very clear line-by-line:

const { title, description, timeRestore, options } = currentState; savedDashboard = { ...savedDashboard, title, description, timeRestore, optionsJSON: JSON.stringify(options), };

This is very old code which was only moved in this PR. Also, since savedDashboard is a class instance, the spread and reassign technique doesn't work here. That said, I've done a bit of cleanup here to make a little nicer to look at.

clintandrewhall · 2021-05-26T13:58:59Z

src/plugins/dashboard/public/application/lib/sync_dashboard_container_input.ts

+
+    const newSearchSessionId: string | undefined = (() => {
+      // do not update session id if this is irrelevant state change to prevent excessive searches
+      if (!shouldRefetch) return;


eslint should have caught/fixed this... do you have it enabled for files in your IDE?

Not sure if this is a rule or not - I've seen it used a bunch of times.

clintandrewhall · 2021-05-26T13:59:17Z

src/plugins/dashboard/public/application/lib/sync_dashboard_container_input.ts

+      // do not update session id if this is irrelevant state change to prevent excessive searches
+      if (!shouldRefetch) return;
+
+      const sessionApi = services.data.search.session;


clintandrewhall · 2021-05-26T14:00:38Z

src/plugins/dashboard/public/application/lib/sync_dashboard_container_input.ts

+  'expandedPanelId',
+  'isFullScreenMode',
+  'isEmbeddedExternally',
+];


Pro tip: add a const assertion here.

https://blog.logrocket.com/const-assertions-are-the-killer-new-typescript-feature-b73451f35802/

clintandrewhall · 2021-05-26T14:00:54Z

src/plugins/dashboard/public/application/lib/sync_dashboard_filter_state.ts

+    data: { query: queryService, search },
+  } = services;
+  const { filterManager, queryString, timefilter } = queryService;
+  const { timefilter: timefilterService } = timefilter;


clintandrewhall · 2021-05-26T14:05:31Z

src/plugins/dashboard/public/types.ts

-  value: string;
-  operator: string;
-  index: string;
+export interface DashboardAppServices {


We should consider structuring this by concern, (though service abstractions may solve this entirely)... we've got event handlers mixed with components mixed with entire service API collections...?

I totally agree about restructuring this. I am saving the refactor of everything services related for our next tech-debt removal session! Very excited to clean this up.

…oardRedux

ThomThomson

@clintandrewhall While I've been making improvements to the dashboard code, it is still extremely dense, and can be overcomplicated at times. This is a step towards fixing that, but there are still plenty more changes to be made.

Additionally, I've certainly been known to value conciseness over readability, and in PRs of this size, that can be killer for reviewers. I've fixed most of your suggestions, and have also made some bigger changes to the structure for simplicities' sake, and added or fleshed out many of the code comments around the dashboard build process. Take a look at the comments below to see what I've changed.

Beyond the simplifications I've done - the services abstraction will absolutely help to mitigate some of this complexity, and further down the line I'd like to move all of the dashboard state inside the embeddable, so increase portability of the system.

ThomThomson · 2021-05-28T21:17:27Z

src/plugins/dashboard/public/application/hooks/use_dashboard_app_state.ts

+  /**
+   * Unpack services
+   */
+  const services = useKibana<DashboardAppServices>().services;


Here, I've moved the unpacking of the services outside the useEffect. This has the downside of adding far more individual items to the deps array, but stops the useEffect from monitoring unnecessary services.

ThomThomson · 2021-05-28T21:20:39Z

src/plugins/dashboard/public/application/hooks/use_dashboard_app_state.ts

+     * The dashboard build context is a collection of all of the services and props required in subsequent steps to build the dashboard
+     * from the dashboardId. This build context doesn't contain any extrenuous services.
+     */
+    const dashboardBuildContext: DashboardBuildContext = {


I've changed the dashboard build context to include all required services unpacked, rather than as part of a services object. This means no more "restating the chain" in the functions down the line.

ThomThomson · 2021-05-28T21:24:04Z

src/plugins/dashboard/public/application/lib/build_dashboard_container.ts

+  });
+
+  /**
+   * Handle the Incoming Embeddable Part 1:


Added some more thorough comments explaining how the incoming embeddable is added to the dashboard. This can be quite confusing.

…oardRedux

kibanamachine · 2021-06-07T19:15:21Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	218	265	+47

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`dashboard`	132	133	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	140.0KB	221.4KB	+81.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dashboard`	327.9KB	325.8KB	-2.2KB
`embeddable`	179.0KB	179.1KB	+48.0B
total			-2.1KB

Unknown metric groups

API count

id	before	after	diff
`dashboard`	144	145	+1

History

💚 Build #129282 succeeded f9b828a
💔 Build #128973 failed 3f18fba
💚 Build #128717 succeeded d684de5
💔 Build #128698 failed d365721
💛 Build #128060 was flaky 60fd0b2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Rebuilt dashboard state management system with RTK.

ThomThomson added 3 commits April 19, 2021 10:55

Rebuilding dashboard state with RTK. Most features complete

6c3df10

Fixed clone code to avoid read only. Changed DashboardState to use a …

a5fe8fb

…panel map for better Diffing. Discard Unsaved Changes

Removed unused translations. Fixed attribute service getInputAsRefSta…

4cef541

…te, cleaned up jest tests

ThomThomson mentioned this pull request Apr 23, 2021

[Dashboard] Capture & Communicate Panel Migrations #98087

Closed

ThomThomson added 8 commits April 26, 2021 13:32

type fixes & Test fixed before merge upstream

156f8af

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

6a22890

…oardRedux

Better diffing for options and panels

a5d47d7

moved onQuerySubmit into dashboard top nav, fixed types, fixed query …

bb5bb67

…sync getting old state

removed unused savedQueryUpdated code

6f33fba

enable stay in edit mode on save as

7ef39f0

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

6585e65

…oardRedux

update yarn lock

46dbda5

ThomThomson force-pushed the feature/dashboardRedux branch from 9a42f37 to 46dbda5 Compare April 28, 2021 20:42

Wrote Jest tests for use dashboard app state. Used replace to prevent…

e996de8

… multiple history records when removing state from URL

ThomThomson force-pushed the feature/dashboardRedux branch 2 times, most recently from f0d596e to 593ba7c Compare May 4, 2021 00:21

ThomThomson changed the title ~~Feature/dashboard redux~~ [Dashboard] Rebuild State Management May 4, 2021

ThomThomson mentioned this pull request May 4, 2021

[Dashboard] Disable color sync by default #99190

Closed

ThomThomson added 3 commits May 4, 2021 17:14

renamed load_dashboard_url_state. fixes for functional test failures

7d06010

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

0714506

…oardRedux

always restore panels in session. Fix maps test

c18a976

ThomThomson force-pushed the feature/dashboardRedux branch from 9351641 to c18a976 Compare May 4, 2021 22:56

nreese approved these changes May 19, 2021

View reviewed changes

ThomThomson added 2 commits May 19, 2021 13:06

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

5dc8329

…oardRedux

Fix missing provider from merge

17b7de9

Merge branch 'master' into feature/dashboardRedux

bd62d72

Merge branch 'master' into feature/dashboardRedux

6c23f35

clintandrewhall approved these changes May 26, 2021

View reviewed changes

ThomThomson added 4 commits May 27, 2021 18:39

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

b7449b4

…oardRedux

More comments and restructuring

0f801a9

Move buildContext inside use effect

60fd0b2

More comments

8e57524

ThomThomson commented May 28, 2021

View reviewed changes

ThomThomson added 6 commits June 2, 2021 11:55

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

d365721

…oardRedux

eslint fix

d684de5

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

3f18fba

…oardRedux

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

d9b2367

…oardRedux

update dashboard_state test

f9b828a

Merge branch 'master' of github.com:elastic/kibana into feature/dashb…

769d65a

…oardRedux

ThomThomson merged commit b3ed014 into elastic:master Jun 7, 2021

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Jun 7, 2021

[Dashboard] Rebuild State Management (elastic#97941)

5ca5273

* Rebuilt dashboard state management system with RTK.

ThomThomson mentioned this pull request Jun 7, 2021

[7.x] [Dashboard] Rebuild State Management (#97941) #101524

Merged

ThomThomson added a commit that referenced this pull request Jun 7, 2021

[Dashboard] Rebuild State Management (#97941) (#101524)

f6f02ae

* Rebuilt dashboard state management system with RTK.

ThomThomson mentioned this pull request Jun 14, 2021

[RFC] Deprecate Dashboard Snapshot URLs in 8.X #102133

Closed

ThomThomson mentioned this pull request Oct 29, 2021

Changing the timepicker resets the dirty status of a vis on dashboards #22876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Rebuild State Management #97941

[Dashboard] Rebuild State Management #97941

ThomThomson commented Apr 22, 2021 •

edited

Loading

flash1293 commented May 5, 2021

nreese left a comment

ThomThomson commented May 19, 2021

ThomThomson commented May 25, 2021

clintandrewhall left a comment

clintandrewhall May 26, 2021

ThomThomson May 27, 2021

clintandrewhall May 26, 2021

ThomThomson May 27, 2021

clintandrewhall May 26, 2021 •

edited

Loading

ThomThomson May 28, 2021 •

edited

Loading

clintandrewhall May 26, 2021

ThomThomson May 27, 2021

clintandrewhall May 26, 2021 •

edited

Loading

ThomThomson May 28, 2021 •

edited

Loading

clintandrewhall May 26, 2021

ThomThomson May 28, 2021

clintandrewhall May 26, 2021

clintandrewhall May 26, 2021

ThomThomson May 28, 2021

clintandrewhall May 26, 2021

clintandrewhall May 26, 2021

ThomThomson May 27, 2021

ThomThomson left a comment

ThomThomson May 28, 2021

ThomThomson May 28, 2021

ThomThomson May 28, 2021

kibanamachine commented Jun 7, 2021

API count

[Dashboard] Rebuild State Management #97941

[Dashboard] Rebuild State Management #97941

Conversation

ThomThomson commented Apr 22, 2021 • edited Loading

Summary

Architecture Diagram

User Facing Changes

Full Unsaved State Persistence

No more View Mode with Unsaved Changes

Better change checking for Panels

Better change checking for Options

Stay in Edit mode on save as

Silent Panel Migrations

Developer Experience Changes

Known Issues

Checklist

For maintainers

flash1293 commented May 5, 2021

nreese left a comment

Choose a reason for hiding this comment

ThomThomson commented May 19, 2021

ThomThomson commented May 25, 2021

clintandrewhall left a 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

clintandrewhall May 26, 2021 • edited Loading

Choose a reason for hiding this comment

ThomThomson May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall May 26, 2021 • edited Loading

Choose a reason for hiding this comment

ThomThomson May 28, 2021 • 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

ThomThomson left a 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

kibanamachine commented Jun 7, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

ThomThomson commented Apr 22, 2021 •

edited

Loading

clintandrewhall May 26, 2021 •

edited

Loading

ThomThomson May 28, 2021 •

edited

Loading

clintandrewhall May 26, 2021 •

edited

Loading

ThomThomson May 28, 2021 •

edited

Loading