[Discover] Add `getRenderAppWrapper` extension #197556

davismcphee · 2024-10-24T02:13:09Z

Summary

This PR adds a getRenderAppWrapper extension to Discover to support advanced state management use cases. It also includes new documentation for the extension point, and overriding default profile implementations:

wrapper_2.mp4

To test, add the following config to kibana.dev.yml:

discover.experimental.enabledProfiles:
  [
    'example-root-profile',
    'example-solution-view-root-profile',
    'example-data-source-profile',
    'example-document-profile',
  ]

Then ingest demo logs and run this in dev tools:

POST _aliases
{
  "actions": [
    {
      "add": {
        "index": "kibana_sample_data_logs",
        "alias": "my-example-logs"
      }
    }
  ]
}

Flaky test runs:

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This will appear in the Release Notes and follow the guidelines

davismcphee · 2024-10-24T02:14:10Z

/ci

kibanamachine · 2024-10-24T02:59:45Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7238

[❌] test/functional/apps/discover/context_awareness/config.ts: 0/25 tests passed.

see run history

logeekal

I gave it a whirl and I have 2 comments to make.

In your previous PR, you had made RootProfileProvider a generic which helped consumers to shape the context returned by resolve method as seen here : https://github.com/elastic/kibana/pull/192934/files#diff-d3059d042d4db0625d2c4d959d7177df794b00c0c7d8ba4f61b47c12e50c92ceR48
Additionaly, in your previous PR, you had context genrated from resolve available in the extension point which is not available as of now. Do you think that can be added back?

Thanks.

davismcphee · 2024-10-28T04:24:31Z

@logeekal I still intend to add support for consumers to extend the returned context, but I'm going to do it as a separate PR. I'd like to keep the changes for the provider work isolated, and I was having trouble identifying the source of test failures in the original PR so I split this work out.

But since you raised it and to better understand your use case, I'm curious how you were planning to use the extended context anyway?

logeekal · 2024-10-28T08:02:31Z

But since you raised it and to better understand your use case, I'm curious how you were planning to use the extended context anyway?

Here is the usage based off of your preview PR: https://github.com/elastic/kibana/pull/197042/files#diff-a0633bbd549f96a17a31909f6f6d2d67a25fd9b65d9a753077a0ebaa373037b9R74

Also, since the process of getting the app context is async and currently I cannot do that in resolve method, I think other methods will be somewhat hacky. I will look into it and let you know.

…llow at root profile level

kibanamachine · 2024-10-28T20:57:57Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7289

[❌] test/functional/apps/discover/context_awareness/config.ts: 24/25 tests passed.

see run history

davismcphee · 2024-10-29T01:07:47Z

@logeekal Thanks for the link, this is along the lines of what I was thinking. It's good to see an example of how a consumer would actually use it. I do intend to add the custom context support as a followup though, so you'll be able to do this.

Also, since the process of getting the app context is async and currently I cannot do that in resolve method, I think other methods will be somewhat hacky. I will look into it and let you know.

The resolve method is async so you should be able to initialize things in it, but maybe I'm just misunderstanding what you mean here.

kibanamachine · 2024-10-29T01:23:06Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7291

[✅] test/functional/apps/discover/context_awareness/config.ts: 25/25 tests passed.

see run history

davismcphee · 2024-10-29T03:52:37Z

src/plugins/discover/public/context_awareness/profiles_manager.ts

+  private getRootRenderAppWrapper() {
+    const rootProfile = this.rootProfileService.getProfile(this.rootContext$.getValue());
+    return rootProfile.getRenderAppWrapper;
+  }


The getRenderAppWrapper extension works a bit differently than other extensions. It can only be used at the root profile level and can only be executed once on root profile resolution. This is because we cannot change the wrapping component at runtime (e.g. when data source profiles resolve) without remounting the entire Discover application (since React cannot reparent components), causing things like loss of local state and reloading of the Lens embeddable. I believe this is an acceptable restriction for this type of extension.

davismcphee · 2024-10-29T03:54:40Z

...discover/public/context_awareness/profile_providers/example/example_root_profile/profile.tsx

+  },
+});
+
+export const createExampleFallbackRootProfileProvider = (): RootProfileProvider => ({


I introduced a separate example-fallback-root-profile which activates when in Solution contexts, in order for us to support functional tests in serverless without breaking all of the existing tests. Open to suggestions if someone has a better alternative.

example-serverless-only-root-profile, example-solution-view-root-profile, or example-single-solution-root-profile

I like example-solution-view-root-profile 👍 Updated here: 2eede18.

elasticmachine · 2024-10-29T03:55:50Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta · 2024-10-29T16:00:57Z

src/plugins/discover/public/context_awareness/hooks/use_root_profile.tsx

+        tap(() => setRootProfileState({ rootProfileLoading: true })),
+        switchMap((id) => profilesManager.resolveRootProfile({ solutionNavId: id })),
+        tap(({ getRenderAppWrapper }) => {
+          const BaseAppWrapper: Profile['getRenderAppWrapper'] = ({ children }) => <>{children}</>;


Can we move the definition of this component out of the hook logic?

Sure we can, updated here: 21ae097.

jughosta · 2024-10-30T15:00:56Z

Sorry, there are conflicts with main now.

davismcphee · 2024-10-30T17:27:45Z

Sorry, there are conflicts with main now.

Fixed now 👍

jughosta

The code changes look good to me and it works great on Discover and on Surrounding Documents page. But something is missing in the embeddable implementation as it does not open the example flyout after pressing a link.

davismcphee · 2024-11-01T13:22:23Z

But something is missing in the embeddable implementation as it does not open the example flyout after pressing a link.

🤦 you're completely right... I even had a note written down to add this and completely overlooked it. I'll update the PR today with embeddable support.

davismcphee · 2024-11-01T19:01:16Z

@jughosta Updated to support the saved search embeddable, and added tests for it: 52aa645.

Also just a note for future reference: I originally tried to migrate to useRootProfile directly in the embeddable, but ran into an issue since data fetching (and therefore data source profile resolution) lives outside React for the embeddable, and root profile resolution must happen first to ensure the rootContext passed to the data source provider's resolve method is accurate. In the future we may want to consider refactoring embeddable fetching to make this easier to manage.

jughosta

LGTM 👍

davismcphee · 2024-11-05T02:59:59Z

/ci

elasticmachine · 2024-11-05T23:35:43Z

💚 Build Succeeded

Buildkite Build
Commit: bb3958a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	969	970	+1

Async chunks

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

id	before	after	diff
`discover`	807.7KB	809.2KB	+1.5KB

Page load bundle

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

id	before	after	diff
`discover`	50.5KB	50.7KB	+195.0B

History

💚 Build #248584 succeeded 8513b75
💛 Build #248331 was flaky 88d6bf9
💔 Build #248323 failed 88d6bf9
💚 Build #247825 succeeded 52aa645
💚 Build #247770 succeeded f27a7d1
💛 Build #247706 was flaky 38aa7be

cc @davismcphee

kibanamachine · 2024-11-06T00:48:51Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11695091320

## Summary This PR adds a `getRenderAppWrapper` extension to Discover to support advanced state management use cases. It also includes new documentation for the extension point, and overriding default profile implementations: https://github.com/user-attachments/assets/70633cbb-1cfe-47fe-984e-ba8afb18fc90 To test, add the following config to `kibana.dev.yml`: ```yml discover.experimental.enabledProfiles: [ 'example-root-profile', 'example-solution-view-root-profile', 'example-data-source-profile', 'example-document-profile', ] ``` Then ingest demo logs and run this in dev tools: ``` POST _aliases { "actions": [ { "add": { "index": "kibana_sample_data_logs", "alias": "my-example-logs" } } ] } ``` Flaky test runs: - 🔴 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7238 - 🔴 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7289 - 🟢 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7291 - x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7303 ### Checklist - [ ] 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] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Julia Rechkunova <[email protected]> (cherry picked from commit 4a95eec)

kibanamachine · 2024-11-06T00:53:24Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…) (#199074) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Add `getRenderAppWrapper` extension (#197556)](#197556)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis McPhee <[email protected]>

## Summary This PR adds a `getRenderAppWrapper` extension to Discover to support advanced state management use cases. It also includes new documentation for the extension point, and overriding default profile implementations: https://github.com/user-attachments/assets/70633cbb-1cfe-47fe-984e-ba8afb18fc90 To test, add the following config to `kibana.dev.yml`: ```yml discover.experimental.enabledProfiles: [ 'example-root-profile', 'example-solution-view-root-profile', 'example-data-source-profile', 'example-document-profile', ] ``` Then ingest demo logs and run this in dev tools: ``` POST _aliases { "actions": [ { "add": { "index": "kibana_sample_data_logs", "alias": "my-example-logs" } } ] } ``` Flaky test runs: - 🔴 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7238 - 🔴 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7289 - 🟢 x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7291 - x25: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7303 ### Checklist - [ ] 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] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Julia Rechkunova <[email protected]>

davismcphee self-assigned this Oct 24, 2024

logeekal reviewed Oct 25, 2024

View reviewed changes

davismcphee added 6 commits October 28, 2024 12:29

Add getRenderAppWrapper extension

f544816

Add functional tests

96279fd

Fix useRootProfile retrieving solutionNavId

3cd9fbd

Fix issue where AppWrapper was remounting entire Discover app, only a…

651a31c

…llow at root profile level

Omit getRenderAppWrapper from data source profile

2361172

Fixing functional tests

16c33cd

davismcphee force-pushed the discover-get-app-wrapper-extension branch from aa6217e to 16c33cd Compare October 28, 2024 15:29

Fixing functionals

0c23d42

davismcphee commented Oct 29, 2024

View reviewed changes

davismcphee marked this pull request as ready for review October 29, 2024 03:55

davismcphee requested review from a team as code owners October 29, 2024 03:55

jughosta reviewed Oct 29, 2024

View reviewed changes

Merge branch 'main' into discover-get-app-wrapper-extension

0675b8d

Merge branch 'main' into discover-get-app-wrapper-extension

608b610

Kerry350 approved these changes Oct 31, 2024

View reviewed changes

davismcphee and others added 3 commits October 31, 2024 21:30

Merge branch 'main' into discover-get-app-wrapper-extension

78ddc8d

Merge branch 'main' into discover-get-app-wrapper-extension

d238f7a

Merge branch 'main' into discover-get-app-wrapper-extension

38aa7be

jughosta reviewed Nov 1, 2024

View reviewed changes

davismcphee added 2 commits November 1, 2024 10:50

Merge branch 'main' into discover-get-app-wrapper-extension

f27a7d1

Support getRenderAppWrapper in dashboards

52aa645

jughosta approved these changes Nov 4, 2024

View reviewed changes

Merge branch 'main' into discover-get-app-wrapper-extension

88d6bf9

efegurkan approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'main' into discover-get-app-wrapper-extension

8513b75

davismcphee added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Nov 5, 2024

Merge branch 'main' into discover-get-app-wrapper-extension

bb3958a

davismcphee merged commit 4a95eec into elastic:main Nov 6, 2024
23 checks passed

kibanamachine added the v9.0.0 label Nov 6, 2024

kibanamachine mentioned this pull request Nov 6, 2024

[8.x] [Discover] Add `getRenderAppWrapper` extension (#197556) #199074

Merged

kibanamachine added the v8.17.0 label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add `getRenderAppWrapper` extension #197556

[Discover] Add `getRenderAppWrapper` extension #197556

davismcphee commented Oct 24, 2024 •

edited by kibanamachine

Loading

davismcphee commented Oct 24, 2024

kibanamachine commented Oct 24, 2024

logeekal left a comment

davismcphee commented Oct 28, 2024

logeekal commented Oct 28, 2024 •

edited

Loading

kibanamachine commented Oct 28, 2024

davismcphee commented Oct 29, 2024

kibanamachine commented Oct 29, 2024

davismcphee Oct 29, 2024

davismcphee Oct 29, 2024

jughosta Oct 29, 2024

davismcphee Oct 29, 2024

elasticmachine commented Oct 29, 2024

jughosta Oct 29, 2024

davismcphee Oct 29, 2024

jughosta commented Oct 30, 2024

davismcphee commented Oct 30, 2024

jughosta left a comment •

edited

Loading

davismcphee commented Nov 1, 2024

davismcphee commented Nov 1, 2024

jughosta left a comment

davismcphee commented Nov 5, 2024

elasticmachine commented Nov 5, 2024

kibanamachine commented Nov 6, 2024

kibanamachine commented Nov 6, 2024

[Discover] Add getRenderAppWrapper extension #197556

[Discover] Add getRenderAppWrapper extension #197556

Conversation

davismcphee commented Oct 24, 2024 • edited by kibanamachine Loading

Summary

Checklist

For maintainers

davismcphee commented Oct 24, 2024

kibanamachine commented Oct 24, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7238

logeekal left a comment

Choose a reason for hiding this comment

davismcphee commented Oct 28, 2024

logeekal commented Oct 28, 2024 • edited Loading

kibanamachine commented Oct 28, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7289

davismcphee commented Oct 29, 2024

kibanamachine commented Oct 29, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7291

davismcphee Oct 29, 2024

Choose a reason for hiding this comment

davismcphee Oct 29, 2024

Choose a reason for hiding this comment

jughosta Oct 29, 2024

Choose a reason for hiding this comment

davismcphee Oct 29, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 29, 2024

jughosta Oct 29, 2024

Choose a reason for hiding this comment

davismcphee Oct 29, 2024

Choose a reason for hiding this comment

jughosta commented Oct 30, 2024

davismcphee commented Oct 30, 2024

jughosta left a comment • edited Loading

Choose a reason for hiding this comment

davismcphee commented Nov 1, 2024

davismcphee commented Nov 1, 2024

jughosta left a comment

Choose a reason for hiding this comment

davismcphee commented Nov 5, 2024

elasticmachine commented Nov 5, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kibanamachine commented Nov 6, 2024

kibanamachine commented Nov 6, 2024

💚 All backports created successfully

Questions ?

[Discover] Add `getRenderAppWrapper` extension #197556

[Discover] Add `getRenderAppWrapper` extension #197556

davismcphee commented Oct 24, 2024 •

edited by kibanamachine

Loading

logeekal commented Oct 28, 2024 •

edited

Loading

jughosta left a comment •

edited

Loading