Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Embeddables Rebuild] Clone panels with runtime state #186052

Merged

Conversation

ThomThomson
Copy link
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
Contributor Author

/ci

@ThomThomson ThomThomson self-assigned this Jun 12, 2024
@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas 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
Contributor Author

/ci

@ThomThomson
Copy link
Contributor Author

/ci

@ThomThomson
Copy link
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
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
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.

@@ -69,6 +74,7 @@ export const apiHasInPlaceLibraryTransforms = (
};

/**
* @deprecated use HasInPlaceLibraryTransforms instead
Copy link
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
Collaborator

💛 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
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
23 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit 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 commit 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 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Embeddables Rebuild] Cloning with runtime state
6 participants