Compute dashboard panel selection list lazily #187797

eokoneyo · 2024-07-08T19:49:49Z

Summary

This PR changes how the dashboard panel selection items get computed, it had previously been computed eagerly, in this implementation panel selection items would only be computed when the user actually clicks the add panel button, with it's results cached so that subsequent interactions with the add panel button leverages the already computed data.

Notable Mention:
The options presented as the dashboard panel list now only comprise of uiActions specifically registered with the uiAction trigger ADD_PANEL_TRIGGER and specific dashboard visualisation types. See #187797 (comment) to follow the reasoning behind this.

That been said adding new panels to the dashboard, would be something along the following lines;

import { ADD_PANEL_TRIGGER } from '@kbn/ui-actions-plugin/public';

 uiActions.attachAction(ADD_PANEL_TRIGGER, <registredActionId>);

// alternatively
// uiActions.addTriggerAction(ADD_PANEL_TRIGGER, ...someActionDefintion);

Visuals

Untitled.mov

How to test

Navigate to a dashboard of choice
Slow down your network speed using your browser dev tools, refresh your dashboard, and click on the “Add panel” button as soon as it is available (before the panels have a chance to load).
You should be presented with a loading indicator, that eventually is swapped out for the list of panels available for selection.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

eokoneyo · 2024-07-08T20:18:02Z

/ci

eokoneyo · 2024-07-09T12:05:36Z

/ci

eokoneyo · 2024-07-10T14:45:38Z

/ci

elasticmachine · 2024-07-10T16:40:53Z

Pinging @elastic/kibana-presentation (Team:Presentation)

nickpeihl · 2024-07-16T14:34:38Z

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx

+                        ...panelGroup,
+                        items: sortGroupPanelsByOrder<PanelSelectionMenuItem>(
+                          (panelGroup.items ?? []).concat(
+                            // TODO: actually add grouping to vis type alias so we wouldn't randomly display an unintended item


Is this TODO expected to be accomplished in this PR or in a separate PR?

I'd say it outside the scope this PR, it would be handled in a different PR.

We're also very close to removing Vis type aliases from this menu entirely, as the new Embeddable system uses only the ADD_PANEL_TRIGGER trigger to determine what shows up here.

src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx

...ns/dashboard/public/dashboard_app/top_nav/add_new_panel/dashboard_panel_selection_flyout.tsx

nickpeihl · 2024-07-17T16:04:43Z

src/plugins/dashboard/public/dashboard_app/top_nav/add_new_panel/use_get_dashboard_panels.ts

+              }
+            });
+
+            return _factoryGroupMap;


Interestingly, even with a platinum license, I'm not seeing any factories returned from the groupUnwrappedEmbeddableFactoriesMap. Many embeddables have migrated to React embeddables which do not apply here. The remaining class based embeddables are excluded by other conditionals, e.g. Lens canCreateNew returns false, Controls are containers, visualize exclusion is hard-coded.

Unless I'm missing something, we might be able to completely remove this function. But maybe we should wait until all class-based embeddables are removed? @nreese @ThomThomson what do you think?

Great research! I think we should be removing this kind of dead-code as soon as we possibly can.

@eokoneyo and I discussed offline and we agreed to remove this groupUnwrappedEmbeddableFactoriesMap$ function from the code. There are currently only eight embeddables that are returned from the getEmbeddablesFactories method.

dashboard - which is a container so it would be filtered out

lens - which is filtered out because canCreateNew on the factory returns false.

search - is also filtered out because canCreateNew returns false.

visualization - is hard-coded to be filtered out

controlGroup - is a container so it is filtered out

6 through 8. optionsList, rangeSlider, timeSlider which all return false from the canCreateNew method.

Additionally, all of these embeddables are in the process of being converted to React embeddables which are registered with the Add panel flyout using triggers. It is highly unlikely that Lens or Search embeddables will change the return value of their canCreateNew methods before the conversion to React embeddables is complete. So I am confident in removing this legacy function.

elasticmachine · 2024-07-19T16:44:49Z

💔 Build Failed

Buildkite Build
Commit: 1a90316

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #34 / dashboard app - group 6 dashboard add ES|QL chart should add an ES|QL datatable chart when the ES|QL panel action is clicked
[job] [logs] FTR Configs #34 / dashboard app - group 6 dashboard add ES|QL chart should add an ES|QL datatable chart when the ES|QL panel action is clicked

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	614	616	+2

Async chunks

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

id	before	after	diff
`dashboard`	533.2KB	532.8KB	-375.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`dashboard`	10	9	-1

References to deprecated APIs

id	before	after	diff
`dashboard`	48	47	-1

Total ESLint disabled count

id	before	after	diff
`dashboard`	10	9	-1

History

💚 Build #222060 succeeded 0a1a981
💛 Build #221149 was flaky d77ca3e
💛 Build #220852 was flaky bfe79a5
💚 Build #220790 succeeded fafc436
💛 Build #220651 was flaky 461af98

cc @eokoneyo

nickpeihl

lgtm! Thanks for removing the code handling the class-based embeddables. it is far simpler to read and removes a lot of legacy logic that is no longer needed.

code review and tested with both basic and premium licenses to verify the panel types appear correctly.

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action.tsx

eokoneyo · 2024-07-23T10:25:27Z

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action_helpers.ts

+export async function isCreateActionCompatible(
+  core: CoreStart,
+  editorFrameService: EditorFrameService
+) {
+  let isCompatible: boolean = core.uiSettings.get(ENABLE_ESQL);
+
+  if (isCompatible && !getDatasourceMap() && !getVisualizationMap()) {
+    const [visualizationMap, datasourceMap] = await Promise.all([
+      editorFrameService.loadVisualizations(),
+      editorFrameService.loadDatasources(),
+    ]);
+
+    if (visualizationMap && datasourceMap) {
+      setDatasourceMap(datasourceMap);
+      setVisualizationMap(visualizationMap);
+    } else {
+      isCompatible = false;
+    }
+  }
+
+  return isCompatible;


Given this PR introduces change to fetch only UIActions registered with the trigger ADD_PANEL_TRIGGER it resulted in required data dependency that would have previously been made available when the classic lens Embeddable isEditable method gets invoked (v8.14.3/x-pack/plugins/lens/public/embeddable/embeddable_factory.ts#L70-L73) being missing as evident in the failing test above. The above change ensures that the required data is articulated as a dependency of the action and exists before the action's execute method is invoked.

rename function arg to prevent confusion with window.close

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action_helpers.ts

mbondyra

Tested and reviewed for Lens code. LGTM 👌🏼

dej611 · 2024-07-25T10:36:19Z

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action_helpers.ts

+  let isCompatible: boolean = core.uiSettings.get(ENABLE_ESQL);
+
+  if (isCompatible && (!getDatasourceMap() || !getVisualizationMap())) {
+    const [visualizationMap, datasourceMap] = await Promise.all([
+      editorFrameService.loadVisualizations(),
+      editorFrameService.loadDatasources(),
+    ]);
+
+    if (visualizationMap && datasourceMap) {
+      setDatasourceMap(datasourceMap);
+      setVisualizationMap(visualizationMap);
+    } else {
+      isCompatible = false;
+    }
+  }
+
+  return isCompatible;


Why introduce a side effect in the compatibility check?
Would it be best to move it within the execution flow?

My thought process here is, preemptively verify before execution that we'd have the data required to add the panel. Should this instead run at the execution point and we get no data back, the experience would mirror the failing test here.

After speaking with @dej611 on slack we decided it's acceptable to have this side effect in the execution flow and halt the execution flow if it so happens that the required data dependency isn't available. 1017c76 addresses this change.

kibana-ci · 2024-07-25T20:14:08Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 1017c76

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	614	616	+2

Async chunks

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

id	before	after	diff
`dashboard`	533.4KB	533.1KB	-328.0B
`lens`	1.5MB	1.5MB	+188.0B
total			-140.0B

Page load bundle

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

id	before	after	diff
`lens`	50.0KB	50.2KB	+187.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`dashboard`	10	9	-1

References to deprecated APIs

id	before	after	diff
`dashboard`	48	47	-1

Total ESLint disabled count

id	before	after	diff
`dashboard`	10	9	-1

History

💚 Build #223760 succeeded 6e93443
💚 Build #223638 succeeded 9f3258a
💛 Build #223233 was flaky efe7994
💔 Build #223199 failed 956193c
💔 Build #223162 failed fa819ac
💔 Build #222897 failed 495f2b8

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

cc @eokoneyo

dej611

Tested locally on Safari and it works 👍

eokoneyo added bug Fixes for quality problems that affect the customer experience Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Project:Dashboard Usability Related to the Dashboard Usability initiative labels Jul 8, 2024

eokoneyo self-assigned this Jul 8, 2024

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from 5a9f898 to b73af33 Compare July 8, 2024 20:01

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from b73af33 to 6b9e897 Compare July 9, 2024 11:59

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from fb6628d to 461af98 Compare July 10, 2024 14:41

eokoneyo marked this pull request as ready for review July 10, 2024 16:40

eokoneyo requested a review from a team as a code owner July 10, 2024 16:40

eokoneyo added the release_note:skip Skip the PR/issue when compiling release notes label Jul 10, 2024

eokoneyo changed the title ~~Compute dashboard panel selection list lazily instead of eagerly~~ Compute dashboard panel selection list lazily Jul 10, 2024

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch 4 times, most recently from d77ca3e to fcf2f79 Compare July 15, 2024 09:17

nickpeihl self-requested a review July 15, 2024 20:43

nickpeihl requested changes Jul 16, 2024

View reviewed changes

ThomThomson reviewed Jul 17, 2024

View reviewed changes

...ns/dashboard/public/dashboard_app/top_nav/add_new_panel/dashboard_panel_selection_flyout.tsx Outdated Show resolved Hide resolved

nickpeihl reviewed Jul 17, 2024

View reviewed changes

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch 2 times, most recently from 2d3e6a2 to 1a90316 Compare July 19, 2024 15:07

nickpeihl approved these changes Jul 19, 2024

View reviewed changes

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch 2 times, most recently from 495f2b8 to 34f09a0 Compare July 23, 2024 09:11

eokoneyo requested a review from a team as a code owner July 23, 2024 09:11

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from 34f09a0 to ffac8d7 Compare July 23, 2024 09:13

eokoneyo commented Jul 23, 2024

View reviewed changes

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action.tsx Outdated Show resolved Hide resolved

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from ffac8d7 to 0550869 Compare July 23, 2024 10:23

eokoneyo commented Jul 23, 2024

View reviewed changes

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch 3 times, most recently from 956193c to efe7994 Compare July 23, 2024 14:32

eokoneyo added 10 commits July 24, 2024 21:19

make call to fetch dashboard panel lazy instead of eager

7d5bd58

handle errors with fetching panels selection list

2a1dd15

add tests for panel flyout component

7b99948

refactor panel approach

9d70fee

rename function arg to prevent confusion with window.close

add tests for useGetDashboardPanels

5bc8caa

restructure code a tad bit

1c66926

use dashboard overlay helper methods

9aadfd7

refactor useGetDashboardPanel to return method that returns a promise

09a3090

leverage only ui actions to build dashboard panels and improve tests

e083693

register data and visualizations for ES|QL when compatible and missing

9f3258a

eokoneyo force-pushed the fix/resolve-187587-lazy-panel-option-loading branch from efe7994 to 9f3258a Compare July 24, 2024 19:20

mbondyra reviewed Jul 25, 2024

View reviewed changes

x-pack/plugins/lens/public/trigger_actions/open_lens_config/create_action_helpers.ts Outdated Show resolved Hide resolved

mbondyra approved these changes Jul 25, 2024

View reviewed changes

use OR in place of AND for data dependecy check

6e93443

dej611 reviewed Jul 25, 2024

View reviewed changes

refactor ES|QL data dependency check to happen in execute method

1017c76

eokoneyo requested a review from dej611 July 26, 2024 08:29

dej611 approved these changes Jul 26, 2024

View reviewed changes

eokoneyo merged commit 6ddffb5 into elastic:main Jul 26, 2024
31 checks passed

eokoneyo deleted the fix/resolve-187587-lazy-panel-option-loading branch July 26, 2024 11:54

kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute dashboard panel selection list lazily #187797

Compute dashboard panel selection list lazily #187797

eokoneyo commented Jul 8, 2024 •

edited

Loading

eokoneyo commented Jul 8, 2024

eokoneyo commented Jul 9, 2024

eokoneyo commented Jul 10, 2024

elasticmachine commented Jul 10, 2024

nickpeihl Jul 16, 2024

eokoneyo Jul 17, 2024

ThomThomson Jul 17, 2024

nickpeihl Jul 17, 2024

ThomThomson Jul 17, 2024

nickpeihl Jul 18, 2024 •

edited

Loading

elasticmachine commented Jul 19, 2024 •

edited

Loading

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

nickpeihl left a comment

eokoneyo Jul 23, 2024 •

edited

Loading

mbondyra left a comment

dej611 Jul 25, 2024

eokoneyo Jul 25, 2024

eokoneyo Jul 26, 2024

kibana-ci commented Jul 25, 2024

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

dej611 left a comment

Compute dashboard panel selection list lazily #187797

Compute dashboard panel selection list lazily #187797

Conversation

eokoneyo commented Jul 8, 2024 • edited Loading

Summary

Visuals

How to test

Checklist

eokoneyo commented Jul 8, 2024

eokoneyo commented Jul 9, 2024

eokoneyo commented Jul 10, 2024

elasticmachine commented Jul 10, 2024

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

nickpeihl Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

nickpeihl left a comment

Choose a reason for hiding this comment

eokoneyo Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

mbondyra 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

kibana-ci commented Jul 25, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

dej611 left a comment

Choose a reason for hiding this comment

eokoneyo commented Jul 8, 2024 •

edited

Loading

nickpeihl Jul 18, 2024 •

edited

Loading

elasticmachine commented Jul 19, 2024 •

edited

Loading

eokoneyo Jul 23, 2024 •

edited

Loading