[Embeddable Rebuild] [Saved Search] Migrate saved search embeddable to new embeddable framework

[Heenawter]

Closes #174959

Summary

This PR converts the Saved Search embeddable to the new React embeddable framework. There should not be any changes in user-facing behaviour (except for the intentional change described here) - therefore, testing of this PR should be focused on ensuring that no behaviour is changed and/or broken with this refactor.

Warning

The saved search embeddable takes many forms and so, while I tried my best to test everything thoroughly, it is very, very likely that I missed some test cases due to not being the expert in this area. It is important that @elastic/kibana-data-discovery in particular approaches this PR review with a fine-tooth comb 🙇 Thanks so much.

Notes about the embeddable state:

As part of this refactor, I made three significant changes to how the state is managed:

1. Once the embeddable is being built in buildEmbeddable, the only difference between the runtime state of a by reference and a by value panel is that the by reference one will have three saved object-specific keys: savedObjectId, savedObjectDescription, and savedObjectTitle.

2. Number 1 made it possible for me to "flatten out" the runtime state of the embeddable by removing the attributes key, which makes it easier to access the pieces of state that you need.

3. Previously, the savedSearch element of the Saved Search embeddable object was never modified; instead, changes made to the columns, sort, sample size, etc. from the dashboard were stored in explicitInput. This essentially created two sources of truth.

   With the new embeddable system, we only ever want one source of truth - so, the saved search is now modified directly when making changes from the dashboard. However, in order to keep behaviour consistent with the old embeddable, changes made from the dashboard to a by reference saved search should not modify the underlying saved object (this behaviour will have to change if we ever want inline editing for saved searches, but that is another discussion) - therefore, when serializing the runtime state (which happens when the dashboard is saved), we only serialize state that has changed from the initial state; then, on deserialization, we take this "changed" state and overwrite the state of the saved search with it.

   Note that this only applies to by reference saved searches - with by value saved searches, we don't have to worry about this and can freely modify the state.

I also had to make changes to how the search source is stored in runtime state. Previously, when initializing the embeddable, fetching the saved search saved object also created and returned an unserializable search source object. However, in the new system, runtime state most always be serializable (see #186052) - therefore, I've had to instead use the serialized search source in my runtime state saved search - therefore, I've had to make changes to toSavedSearch method to allow for a serializable saved search to be returned.

	Runtime state (input) before	Runtime state after
By value
By reference

Checklist

• 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
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[Heenawter]

/ci

[Heenawter]

/ci

[davismcphee]

Thanks for addressing the feedback! I tested basically all combinations of saved searches I could think of including data view, ad hoc data view, ES|QL, by ref/value, etc. and it seems to be working great 👍 I noticed a few minor bugs, but they all seem to be in main too so I'll make separate issues for those. There were just two things I noticed that might need to be addressed before I think it's good to merge:

• When I add a by ref search panel in main, the panel title/description sync to the saved search when modified in Discover unless I've manually changed the panel title/description. In this PR the panel title/description don't seem to sync with the saved search at all. Should it still behave how it does in main or is this working as intended?
• When I unlink a search panel from the library in main, it keeps all of the SO properties and passes them to Discover when clicking the edit dropdown link. In this PR it looks like some of the properties are dropped when unlinking, so clicking edit doesn't carry over some state like the breakdown field. I feel like it might be best to keep all of the state if possible in case we decide to use it in the embeddable in the future.

[Heenawter]

@davismcphee

When I add a by ref search panel in main, the panel title/description sync to the saved search when modified in Discover unless I've manually changed the panel title/description. In this PR the panel title/description don't seem to sync with the saved search at all. Should it still behave how it does in main or is this working as intended?

Looks like this is a bug with all by-reference React embeddables, not just saved searches (I just tested with Maps and I am seeing similar behaviour) - would it be okay if we fix this in a follow up, since it is technically unrelated to this PR? I've created an issue here. Our plan is to solve this in a more generic way so we don't see this bug popup in the future.

In this PR it looks like some of the properties are dropped when unlinking, so clicking edit doesn't carry over some state like the breakdown field.

Interesting - I removed the breakdownField in 72c6f7c because @jughosta noted that it wasn't used for the embeddable. Do we want to keep state around even if the embeddable doesn't use it? Seems like we have two conflicting opinions here, hahah :) I can go whichever way ya'll would prefer!! But perhaps this isn't a blocker in that case?

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 56d65a8

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
discover	953	957	+4
reporting	117	151	+34
savedSearch	67	23	-44
total			-6

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
discover	97	102	+5
savedSearch	79	60	-19
total			-14

Async chunks

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

id	before	after	diff
cloudSecurityPosture	455.3KB	455.3KB	+25.0B
discover	821.6KB	816.7KB	-4.8KB
esqlDataGrid	116.6KB	116.6KB	+25.0B
securitySolution	17.3MB	17.3MB	+125.0B
slo	877.9KB	878.0KB	+25.0B
total			-4.6KB

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
@kbn/reporting-csv-share-panel	1	0	-1
discover	29	26	-3
savedSearch	4	3	-1
total			-5

Page load bundle

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

id	before	after	diff
discover	43.8KB	52.6KB	+8.8KB
embeddable	71.3KB	71.3KB	-6.0B
reporting	52.9KB	53.9KB	+1.0KB
savedSearch	11.5KB	11.4KB	-103.0B
total			+9.8KB

Unknown metric groups

API count

id	before	after	diff
discover	144	149	+5
savedSearch	80	61	-19
total			-14

ESLint disabled line counts

id	before	after	diff
discover	26	24	-2

References to deprecated APIs

id	before	after	diff
discover	17	9	-8
savedSearch	12	11	-1
total			-9

Total ESLint disabled count

id	before	after	diff
discover	28	26	-2

History

• 💚 Build #222713 succeeded 0ece8d7
• 💔 Build #222706 failed 229d241
• 💔 Build #222689 failed 9e24c2d
• 💚 Build #218313 succeeded 4362c2e
• 💛 Build #217344 was flaky 53580df
• 💛 Build #216973 was flaky 8169f8d

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

cc @Heenawter

[davismcphee]

Looks like this is a bug with all by-reference React embeddables, not just saved searches (I just tested with Maps and I am seeing similar behaviour) - would it be okay if we fix this in a follow up, since it is technically unrelated to this PR?

Yes sounds great, thanks!

Interesting - I removed the breakdownField in 72c6f7c because @jughosta noted that it wasn't used for the embeddable. Do we want to keep state around even if the embeddable doesn't use it?

Hmm I agree that we don't need the breakdown field as part of the embeddable runtime state since it's not currently used by the embeddable, but I would expect unlinking a panel from the library to clone and persist the linked SO in its entirety, not just the runtime state the embeddable uses. That way all of the properties would be there if the embeddable is later updated to use them, plus actions like "Open in Discover" would include all search properties, which can be useful when starting a Discover investigation from a Dashboard panel (similarly save & return functionality wouldn't result in lost state if an embeddable doesn't use it directly).

Maybe this is different from how the Presentation team thinks about it, but it felt unexpected to me when testing. That said, I don't think it's a major issue and agree it doesn't need to block this PR. I'll add it to #188550 for us to look into as a followup.

And in that case this LGTM, thanks for all the work on it! I know it was a huge task and we greatly appreciate it 🙏 I'm approving now, but I just have one more tiny request if possible before merging 😅 It looks like the Discover page load bundle increased 20% from this PR. Is there a way we could knock this down some before merging, maybe with dynamic imports? We try our best to keep the page load bundle as small as possible when we can.

[Heenawter]

It looks like the Discover page load bundle increased 20% from this PR. Is there a way we could knock this down some before merging, maybe with dynamic imports? We try our best to keep the page load bundle as small as possible when we can.

Sorry yeah, this is 100% something we are aware of. The +8.8KB increase is specifically coming from the @kbn/presentation-containers package - at one point, we were looking at making this a shared package so that each plugin wouldn't inherit this bloat, but at the time it was deemed not worth it (@ThomThomson would know more about this, since he is the one that looked into it).

This is definitely something on our mind (since all React embeddables are dealing with this), so stay tuned for changes here - but unfortunately will have to merge this PR as-is because that will be a larger discussion with the team 🙈 Hopefully that is okay!!

cc @nreese

[davismcphee]

@Heenawter No problem! Thanks for the explanation and glad to hear it's on your radar already 🙂