[Embeddable] Refactor embeddable panel by ThomThomson · Pull Request #159837 · elastic/kibana

ThomThomson · 2023-06-15T19:12:10Z

Summary

This PR accomplishes a number of tasks related to the Embeddable Panel. This PR:

Updates the Embeddable panel and all sub-components to be a react component
Removes the services props in favour of a simple services provider in the embeddable plugin.
Removes the embeddable panel HOC
Allows access to the Embeddable panel via direct import rather than via the plugin start contract.
Simplifies all usages of the embeddable panel.
Shows one unified panel loading state when a panel is loading.

Closes #140408
Closes #148855
Closes #158977

This PR also unblocks #158677 by providing a space where the embeddable migrations can be run client side.

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

…EmbeddablePanel

…fix'

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

…EmbeddablePanel

…fix'

…test. Restore seamless React integration. Remove check before updating badges, notifications, and actions.

…EmbeddablePanel

…isabled

ThomThomson · 2023-07-07T18:04:38Z

@elasticmachine merge upstream

ThomThomson · 2023-07-10T19:06:27Z

@elasticmachine merge upstream

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

nreese · 2023-07-10T20:40:03Z

src/plugins/embeddable/public/embeddable_panel/use_select_from_embeddable.ts

+import { distinctUntilKeyChanged } from 'rxjs';
+import { EmbeddableInput, EmbeddableOutput, IEmbeddable } from '../lib';
+
+export const useSelectFromOptionalEmbeddableInput = <


I am not sure useSelectFromOptionalEmbeddableInput is the best name. Its only used once (see code below), and its not for optional key, its for optional embeddable. How about just removing useSelectFromEmbeddableInput and renaming useSelectFromOptionalEmbeddableInput to useSelectFromEmbeddableInput?

const parentHidePanelTitle = useSelectFromOptionalEmbeddableInput( 'hidePanelTitles', embeddable.parent );

That's not a bad idea - I think I anticipated using that more. One thing that removing useSelectFromOptionalEmbeddableInput will do is mean that any time you select from the embeddable input it'll be type | undefined. I'm not sure if that would cause any problems - probably will just mean some more guards. I'll implement this!

nreese · 2023-07-10T20:42:10Z

src/plugins/embeddable/public/embeddable_panel/embeddable_loading_indicator.tsx

+import React from 'react';
+import { EuiLoadingChart, EuiPanel } from '@elastic/eui';
+
+export const EmbeddableLoadingIndicator = ({ showShadow }: { showShadow?: boolean }) => {


Is showShadow prop needed? Its never used.

It's used once here x-pack/plugins/lens/public/embeddable/embeddable_component.tsx.

nreese · 2023-07-10T20:45:26Z

src/plugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_title.tsx

+import { useSelectFromEmbeddableInput } from '../use_select_from_embeddable';
+import { getEditTitleAriaLabel, placeholderTitle } from '../embeddable_panel_strings';
+
+export const useEmbeddablePanelTitle = (


nit
Why is this a hook vs just a component? It returns an ReactNode or null. I think making it a normal component vs a hook would make more sense.

Good call, will swap this over!

MichaelMarcialis

Code review only. Left one small comment below, but nothing worth holding you up over. Approving.

MichaelMarcialis · 2023-07-11T14:57:04Z

src/plugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_badges.tsx

+          <EuiNotificationBadge
+            data-test-subj={`embeddablePanelNotification-${notification.id}`}
+            key={notification.id}
+            style={{ marginTop: '4px', marginRight: '4px' }}


Should we use EUI variables here instead of the hard-coded 4px?

Good call! Switched that over!

…EmbeddablePanel

…fix'

nreese · 2023-07-12T22:32:20Z

...lugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_context_menu.tsx

+  useEffect(() => {
+    let subscription: Subscription;
+
+    updatePanelActions().then(() => {


This may cause a race condition where useEffect could run again before updatePanelActions returns from the first time useEffect is called. That would set up a subscription that is then never canceled.

You should protect .then path that does not create subscriptions if useEffect is triggered before updatePanelActions returns. This should do the trick

useEffect(() => { let ignore = false; let subscription: Subscription; updatePanelActions().then(() => { if (mounted() && !ignore) { /** * since any piece of state could theoretically change which actions are available we need to * recalculate them on any input change or any parent input change. */ subscription = embeddable.getInput$().subscribe(() => updatePanelActions()); if (embeddable.parent) { subscription.add(embeddable.parent.getInput$().subscribe(() => updatePanelActions())); } } }); return () => { ignore = true; subscription?.unsubscribe(); }; }, [embeddable, getAllPanelActions, updatePanelActions, mounted]);

Good find! I've updated this to match.

nreese · 2023-07-12T22:34:45Z

...lugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_context_menu.tsx

+      className="embPanel__optionsMenuButton"
+      onClick={() => {
+        updatePanelActions().then(() => {
+          if (!mounted()) return;


Should some kind of loading indicator be displayed? You are delaying reacting to user interaction until an async function finishes. If this async function takes more then 100 milliseconds, then users may think there click was not registered. How about changing the icon to loading indicator, or opening menu and displaying EUI skeleton in place of menu until async action finishes.

Also, why is $input subscription needed if updatePanelActions is called when menu is opened? Wouldn't contextMenuPanels already be current and this call not needed?

Good catch! In my testing, the async function always returned pretty instantaneously, but it's always best practice to make a proper loading state anyway. I've updated to show this:

IRT the $input subscription - before these changes, the panel actions were recalculated all the time, so I had this subscription to recreate that behaviour. I've removed it in favour of a call on mount, and a call when the menu is opened, and it seems like it works!

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

nreese · 2023-07-13T19:13:09Z

...lugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_context_menu.tsx

@@ -0,0 +1,174 @@
+/*


Should this just be a component instead of a hook?

Probably! Will update that.

nreese · 2023-07-13T19:16:09Z

src/plugins/embeddable/public/embeddable_panel/panel_header/embeddable_panel_title.tsx

+  embeddable: IEmbeddable;
+  customizePanelAction?: CustomizePanelAction;
+}) => {
+  const title = embeddable.getTitle();


Parent component EmbeddablePanelHeader also sets up subscriptions for title, viewMode, and description. How about just taking these values as props to avoid duplicating state and $input subscriptions here?

I usually like having separate calls to select directly inside the components in which that state gets used. I'm not totally sure if this is a performance issue, but will do some research. I see prop drilling as a bit of a trade-off in that it can occasionally lower readability.

There is a best practice for this in the redux style guide called Connect More Components to Read Data from the Store. This isn't exactly the same, because this is using a subscription from RXJS, but the selector uses some sort of subscription under the hood, so I think it might be similar at least.

nreese · 2023-07-14T15:55:18Z

src/plugins/embeddable/public/embeddable_panel/panel_header/use_embeddable_panel_badges.tsx

+    };
+
+    updateNotificationsAndBadges().then(() => {
+      if (mounted) {


Also check for canceled to avoid creating subcriptions if useEffect is fired before previous async action completes

This variable actually is tracking the canceled state - you can see that on line 99 - I've just called it mounted here instead. I'll rename the variable to be more consistent though!

nreese

LGTM
code review, tested in chrome

ThomThomson · 2023-07-14T18:59:34Z

@elasticmachine merge upstream

kibana-ci · 2023-07-14T20:09:12Z

💛 Build succeeded, but was flaky

Failed CI Steps

Defend Workflows Endpoint Cypress Tests #2

Test Failures

[job] [logs] Defend Workflows Endpoint Cypress Tests #2 / Automated Response Actions From alerts should have generated endpoint and rule

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`embeddable`	84	88	+4

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
`embeddable`	444	434	-10

Async chunks

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

id	before	after	diff
`canvas`	994.1KB	994.1KB	-12.0B
`dashboard`	356.0KB	355.6KB	-401.0B
`embeddable`	0.0B	11.9KB	+11.9KB
`securitySolution`	9.8MB	9.8MB	-12.0B
total			+11.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`embeddable`	4	7	+3

Page load bundle

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

id	before	after	diff
`embeddable`	76.0KB	64.4KB	-11.6KB
`lens`	36.6KB	36.5KB	-74.0B
total			-11.6KB

Unknown metric groups

API count

id	before	after	diff
`embeddable`	548	532	-16

async chunk count

id	before	after	diff
`embeddable`	0	2	+2

References to deprecated APIs

id	before	after	diff
`embeddable`	22	16	-6

History

💛 Build #142400 was flaky 8f238d9
💔 Build #142362 failed 7baab41
💚 Build #141748 succeeded af6a9ce
💔 Build #141385 failed 1bf063f
💛 Build #141364 was flaky f193db7
💔 Build #141340 failed 9fca516

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

stephmilovic

LGTM, tested the security solution implementation and we are all set. Thank you for the updates!

ThomThomson and others added 16 commits April 20, 2023 10:25

initial implementation of new embeddable panel component

f8b41df

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

29e3b7b

…EmbeddablePanel

initial usage in grid item

e1bb98a

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

42412a7

…EmbeddablePanel

update all usages to the new Embeddable Panel

fd53ee6

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

8a5a0e6

…fix'

remove unused translations. Update jest tests

f9f6e1d

Merge remote-tracking branch 'refs/remotes/origin/embeddable/refactor…

06d8819

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

restore open add panel flyout

7b4c123

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

f25ecc8

…EmbeddablePanel

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

30055f9

…fix'

fix types

d39affe

fix error recoverability, refresh actions when opening action menu

ee7e19e

Make add panel flyout use saved object type. Update embeddable panel …

e64c892

…test. Restore seamless React integration. Remove check before updating badges, notifications, and actions.

fix add panel flyout test

5dec26b

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

f3a95a4

…EmbeddablePanel

This was referenced Jul 7, 2023

[dashboard] fix Memory leak when reset causes by-value panel to be deleted #161394

Merged

[Dashboard Navigation] Add creation UI #160179

Merged

ThomThomson added 2 commits July 7, 2023 11:33

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

50986fd

…EmbeddablePanel

re order disabled action filtering to allow universal actions to be d…

1958bd7

…isabled

Merge branch 'main' into embeddable/refactorEmbeddablePanel

8c3d029

ThomThomson marked this pull request as ready for review July 7, 2023 19:18

ThomThomson requested review from a team as code owners July 7, 2023 19:18

ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:large Large Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Jul 7, 2023

kibanamachine and others added 3 commits July 10, 2023 15:06

Merge branch 'main' into embeddable/refactorEmbeddablePanel

da66eac

Review feedback, remove unused props

c95b94f

Merge remote-tracking branch 'refs/remotes/origin/embeddable/refactor…

1bf063f

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

nreese reviewed Jul 10, 2023

View reviewed changes

MichaelMarcialis approved these changes Jul 11, 2023

View reviewed changes

ThomThomson and others added 3 commits July 11, 2023 17:03

Review feedback

c3e247c

Merge remote-tracking branch 'upstream/main' into embeddable/refactor…

655609a

…EmbeddablePanel

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

af6a9ce

…fix'

nreese reviewed Jul 12, 2023

View reviewed changes

ThomThomson added 2 commits July 13, 2023 13:27

panel settings menu loading state.

f89ab81

Merge remote-tracking branch 'refs/remotes/origin/embeddable/refactor…

7baab41

…EmbeddablePanel' into embeddable/refactorEmbeddablePanel

nreese reviewed Jul 13, 2023

View reviewed changes

Review feedback

8f238d9

nreese reviewed Jul 14, 2023

View reviewed changes

mounted -> canceled

c3caa8c

nreese approved these changes Jul 14, 2023

View reviewed changes

Merge branch 'main' into embeddable/refactorEmbeddablePanel

260b67c

stephmilovic approved these changes Jul 17, 2023

View reviewed changes

ThomThomson merged commit a1be033 into elastic:main Jul 17, 2023

kibanamachine added v8.10.0 backport:skip This PR does not require backporting labels Jul 17, 2023

ThomThomson mentioned this pull request Jul 18, 2023

[Embeddables] Fix Notifications show header #162194

Merged

1 task

nickpeihl mentioned this pull request Sep 19, 2023

[Dashboard] Descriptions not showing up on panel after upgrade #166773

Closed

Conversation

ThomThomson commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

ThomThomson commented Jul 7, 2023

Uh oh!

ThomThomson commented Jul 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Jul 14, 2023

Uh oh!

kibana-ci commented Jul 14, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

References to deprecated APIs

History

Uh oh!

stephmilovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

ThomThomson commented Jun 15, 2023 •

edited

Loading