[Alerting v2] Export composable page components from public start contract - #288805
Conversation
…ublic start contract
Adds five lazy-loaded page components (RulesPage, RuleLibraryPage, EpisodesPage,
ActionPoliciesPage, ExecutionHistoryPage) to AlertingV2PublicStart so solution
plugins can mount alerting v2 surfaces at their own routes without duplicating
the DI container, QueryClient, breadcrumb, and i18n provider setup.
Each component accepts { coreStart, setBreadcrumbs } and internally resolves
the Inversify container, creates a fresh QueryClient, and wraps the
corresponding *App component in the full provider stack. EpisodesPage
additionally assembles the KibanaContextProvider service bag from the DI
container, matching the management mount behavior.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Reviewed the composable page exports. The five wrapped components faithfully mirror the existing mount.tsx provider stack (all episode dependencies are requiredPlugins, so the DI container.get calls are safe, and the module is correctly code-split via React.lazy). One inline note on stabilizing the EpisodesPage services bag to avoid unnecessary context re-renders.
Generated by Claude Reviewer for #288805 · opus · 153.1 AIC · ⌖ 14.2 AIC · ⊞ 5.1K
…outer, tab href overrides, and execution history tab rename Fixes discovered during the IA demo: - Inject the alerting v2 DI container via module-level closure instead of coreStart.injection.getContainer() (wrong container scope for cross-plugin consumers) - Wrap each composable page in MemoryRouter so inner *App routes see / as root - Add EuiPageSection paddingSize="m" for management-parity layout - Add TabHrefOverrides context so v1/v2 Rules tab hrefs can be overridden by solution plugins (observability alerting URLs instead of management) - Wire tabs into RulesListHeader via useTabHrefOverrides - Rename execution history "Policies" tab to "Action Policies" - Update tests for new container prop and internal MemoryRouter Co-authored-by: Cursor <cursoragent@cursor.com>
…elated changes Memoize kibanaReactServices in AlertingV2EpisodesPage with useMemo keyed on coreStart/container and hoist Storage to module scope to prevent unnecessary KibanaContextProvider re-renders. Revert rules_list_header tab-wiring and execution_history label rename that depend on packages not yet on main. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Reviewed the new tabHrefOverrides prop and the reworked container/lazy wiring. One inline note on the shared page props advertising tabHrefOverrides on all five exported pages while only RulesPage honors it. The earlier EpisodesPage services-bag memoization concern is now addressed (useMemo + hoisted Storage); requesting resolution of that thread.
Generated by Claude Reviewer for #288805 · opus · 123 AIC · ⌖ 14.4 AIC · ⊞ 5.1K
Tab href overrides will be introduced in a separate PR; keep this change focused on the base composable page exports. Co-authored-by: Cursor <cursoragent@cursor.com>
…t interface Co-authored-by: Cursor <cursoragent@cursor.com>
| <QueryClientProvider client={queryClient}> | ||
| <BreadcrumbProvider setBreadcrumbs={setBreadcrumbs}> | ||
| <I18nProvider> | ||
| <MemoryRouter> |
There was a problem hiding this comment.
shouldn't the consumers be the ones to handle all navigation/routing?
| React.createElement( | ||
| React.Suspense, | ||
| { fallback: null }, | ||
| React.createElement(LazyComponent, { ...props, container: _alertingV2Container! }) |
There was a problem hiding this comment.
can't you just use the container from props?
| React.createElement(LazyComponent, { ...props, container: _alertingV2Container! }) | |
| React.createElement(LazyComponent, { ...props, container: props.coreStart.injection.getContainer() }) |
…I container from props Consumers must provide their own Router ancestor, removing the internal MemoryRouter that fought with the consumer's router context. The module-scoped _alertingV2Container is replaced by props.coreStart.injection.getContainer() per review feedback. Also drops the deprecated RedirectAppLinks wrapper. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Reviewed the composable page exports. The five wrapped pages otherwise faithfully mirror the mount.tsx provider stacks; the earlier memoization and tabHrefOverrides concerns are resolved. One inline note on EpisodesPage dropping the episodes-specific RedirectAppLinks wrapper.
Generated by Claude Reviewer for #288805 · opus · 170.2 AIC · ⌖ 14.4 AIC · ⊞ 5.1K
| <KibanaContextProvider services={kibanaReactServices}> | ||
| <Context.Provider value={container}> | ||
| <QueryClientProvider client={queryClient}> | ||
| <BreadcrumbProvider setBreadcrumbs={setBreadcrumbs}> |
There was a problem hiding this comment.
mountEpisodesApp in mount.tsx wraps <EpisodesApp /> in <RedirectAppLinks coreStart={coreStart}>, but this composable version omits it (the import is also gone, though the test still mocks @kbn/shared-ux-link-redirect-app). None of the other four pages had RedirectAppLinks in mount.tsx — it was episodes-specific — so a consumer can't be assumed to supply it globally. Without it, cross-app anchor links rendered inside the episodes/alert-detail UI (e.g. Discover, doc-viewer links) fall back to full page reloads instead of SPA navigation, a user-visible regression versus the management episodes app. The PR description states EpisodesPage "additionally assembles ... and RedirectAppLinks", which suggests this drop was unintentional. Re-add the wrapper (and its import) around <EpisodesApp />.
There was a problem hiding this comment.
I'm gonna leave out of this PR for now, the redirect and the router stuff is tricky and I need to be able to use it in a consumer to know exactly what I need.
davismcphee
left a comment
There was a problem hiding this comment.
Code-only review, Data Discovery changes LGTM 👍 Just an updated mock.
| services.alertingVTwo = { | ||
| CreateRuleOptionsFlyout: createRuleOptionsFlyoutMock, | ||
| RulesPage: jest.fn(() => null), | ||
| RuleLibraryPage: jest.fn(() => null), | ||
| EpisodesPage: jest.fn(() => null), | ||
| ActionPoliciesPage: jest.fn(() => null), | ||
| ExecutionHistoryPage: jest.fn(() => null), | ||
| }; |
There was a problem hiding this comment.
Nit: long term, it would be better to instead update the shared services mock directly:
kibana/src/platform/plugins/shared/discover/public/__mocks__/services.ts
Lines 334 to 336 in dd0fca8
Not a blocker or anything though.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
shared chunks total size
total optimizer output size
Test Failures
History
|
Summary
Closes elastic/rna-program#1019
Exports five DI-wrapped page components from
AlertingV2PublicStartso solution plugins (e.g. Observability Alerting) can mount alerting v2 surfaces at their own routes without duplicating provider setup.New exports on
AlertingV2PublicStartRulesPageRuleLibraryPageEpisodesPageActionPoliciesPageExecutionHistoryPageProps
Each component internally resolves the Inversify DI container from
coreStart.injection.getContainer(), creates a freshQueryClient, and wraps the corresponding*Appin the full provider stack (Context.Provider,QueryClientProvider,BreadcrumbProvider,I18nProvider).EpisodesPageadditionally assembles theKibanaContextProviderservice bag andRedirectAppLinks.Consumer usage
Files changed
public/application/composable_pages.tsx— new, the five wrapped componentspublic/application/composable_pages.test.tsx— new, 11 testspublic/types.ts— add page types +AlertingV2PagePropsto start contractpublic/index.ts— lazy-load composable pages, wire into start bindingTest plan
Made with Cursor