Skip to content

[Embeddables Rebuild] Clone panels with runtime state#186052

Merged
ThomThomson merged 9 commits intoelastic:mainfrom
ThomThomson:embeddablesRebuild/cloneRuntimeState
Jun 20, 2024
Merged

[Embeddables Rebuild] Clone panels with runtime state#186052
ThomThomson merged 9 commits intoelastic:mainfrom
ThomThomson:embeddablesRebuild/cloneRuntimeState

Conversation

@ThomThomson
Copy link
Copy Markdown
Contributor

@ThomThomson ThomThomson commented Jun 11, 2024

Summary

Fixes #186036 by making the clone operation use runtime state rather than serialized state.

@ThomThomson
Copy link
Copy Markdown
Contributor Author

/ci

@ThomThomson ThomThomson self-assigned this Jun 12, 2024
@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:small Small Level of Effort impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Feature:Embeddables Relating to the Embeddable system project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes labels Jun 12, 2024
@ThomThomson
Copy link
Copy Markdown
Contributor Author

/ci

@ThomThomson
Copy link
Copy Markdown
Contributor Author

/ci

@ThomThomson
Copy link
Copy Markdown
Contributor Author

/ci

@ThomThomson ThomThomson marked this pull request as ready for review June 19, 2024 19:20
@ThomThomson ThomThomson requested a review from a team as a code owner June 19, 2024 19:20
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@ThomThomson ThomThomson requested a review from a team as a code owner June 19, 2024 20:48
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jun 19, 2024
Copy link
Copy Markdown
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review + tested this with my saved search embeddable. Everything seems to work as expected! Tested cloning to ensure it is cloned by value + tested backup storage to ensure that references are passed properly.

};

/**
* @deprecated use HasInPlaceLibraryTransforms instead
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #10 / ManualRuleRunModal should render confirmation button disabled if invalid time range has been selected
  • [job] [logs] Jest Tests #10 / ManualRuleRunModal should render confirmation button disabled if selected end date is in future
  • [job] [logs] Jest Tests #10 / ManualRuleRunModal should render confirmation button disabled if selected start date is more than 90 days in the past

Metrics [docs]

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
@kbn/presentation-publishing 174 177 +3
kibanaUtils 416 417 +1
total +4

Async chunks

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

id before after diff
dashboard 494.3KB 495.0KB +734.0B

Page load bundle

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

id before after diff
embeddable 70.7KB 70.7KB +9.0B
kibanaUtils 71.7KB 71.8KB +49.0B
total +58.0B
Unknown metric groups

API count

id before after diff
@kbn/presentation-publishing 209 212 +3
kibanaUtils 609 610 +1
total +4

References to deprecated APIs

id before after diff
dashboard 50 48 -2
maps 27 31 +4
total +2

History

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

cc @ThomThomson

Copy link
Copy Markdown
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared UX changes in kibana utils LGTM thanks!

@ThomThomson ThomThomson merged commit 7e19cc5 into elastic:main Jun 20, 2024
@kibanamachine kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels Jun 20, 2024
bhapas pushed a commit to bhapas/kibana that referenced this pull request Jun 24, 2024
Makes the clone operation use runtime state rather than serialized state.
Heenawter added a commit that referenced this pull request Jul 22, 2024
…o new embeddable framework (#180536)

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](#180536 (comment)))
- 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](https://github.com/elastic/kibana/pull/180536/files#diff-7346937694685b85c017fb608c6582afb3aded0912bfb42fffa4b32a6d27fdbbR93-R117);
then, on deserialization, we take this "changed" state and
[**overwrite** the state of the saved search with
it](https://github.com/elastic/kibana/pull/180536/files#diff-7346937694685b85c017fb608c6582afb3aded0912bfb42fffa4b32a6d27fdbbR44-R54).
    
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](https://github.com/elastic/kibana/pull/180536/files#diff-3baaeaeef5893a5a4db6379a1ed888406a8584cb9d0c7440f273040e4aa28166R160-R169).

| | Runtime state (`input`) before | Runtime state after |
|--------|--------|--------|
| **By value** |
![image](https://github.com/elastic/kibana/assets/8698078/d019f904-aac3-4bf2-8f9f-a98787d3b78a)
|
![image](https://github.com/elastic/kibana/assets/8698078/dd820202-f1ef-4404-9450-610989204015)
|
| **By reference** |
![image](https://github.com/elastic/kibana/assets/8698078/ebb0d4a9-b918-48a4-8690-0434a2a17561)
|
![image](https://github.com/elastic/kibana/assets/8698078/16fa1e4d-064d-457b-98af-4697f52de4dd)
|


### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Embeddables Relating to the Embeddable system Feature:Embedding Embedding content via iFrame impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Embeddables Rebuild] Cloning with runtime state

6 participants