From a3d379950eea930f1344126c3af35944dc04aa49 Mon Sep 17 00:00:00 2001 From: Maryam Saeidi Date: Wed, 18 Dec 2024 13:59:23 +0100 Subject: [PATCH 1/3] Fix context.pageName by fixing missing executionContext and add enableExecutionContextTracking flag (#204547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves https://github.com/elastic/kibana/issues/195778 ## ๐Ÿž Summary This PR fixes missing executionContext in sharedux router by adding `SharedUXContext` to the `KibanaRootContextProvider` (inside of the `KibanaRenderContextProvider`). (More context provided in this https://github.com/elastic/kibana/issues/195778#issuecomment-2426936142) It also introduces `enableExecutionContextTracking` to enable tracking logic to avoid creating a race condition for the existing custom execution context tracking implementations. I enabled this flag for the observability plugin and here is the difference: |Item|Screenshot| |---|---| |Before|![image](https://github.com/user-attachments/assets/83283d23-3347-45be-95c1-120cdfabb9c5)| |After|![image](https://github.com/user-attachments/assets/9de51645-6bf1-4537-baeb-6878e7bb3590)| ### ๐Ÿงช How to test - Go to the observability alerts page and check the kibana-browser request as shown above ### โœจ Possible future improvements Allowing this logic to be provided by the consumer so that we can get rid of custom implementations (Example: [APM custom execution context](https://github.com/elastic/kibana/blob/e9671937bacfaa911d32de0e8885e7f26425888a/x-pack/plugins/observability_solution/apm/public/components/routing/app_root/update_execution_context_on_route_change.ts#L21,L25)) --------- Co-authored-by: Anton Dosov Co-authored-by: Davis McPhee Co-authored-by: Marco Antonio Ghiani Co-authored-by: Elena Stoeva (cherry picked from commit 53748fdefa1d59d58a4708258a1476dc140b1588) # Conflicts: # packages/react/kibana_context/render/render_provider.tsx # packages/react/kibana_context/root/BUILD.bazel # packages/react/kibana_context/root/root_provider.test.tsx # packages/react/kibana_context/root/root_provider.tsx # packages/react/kibana_context/root/tsconfig.json --- .../kibana_context/render/render_provider.tsx | 8 ++- .../root/root_provider.test.tsx | 19 ++++-- .../kibana_context/root/root_provider.tsx | 17 ++++- .../react/kibana_context/root/tsconfig.json | 6 +- packages/shared-ux/router/impl/BUILD.bazel | 34 ++++++++++ .../impl/__snapshots__/route.test.tsx.snap | 2 + packages/shared-ux/router/impl/index.ts | 2 + packages/shared-ux/router/impl/route.test.tsx | 19 ++++++ packages/shared-ux/router/impl/route.tsx | 16 +++-- packages/shared-ux/router/impl/routes.tsx | 62 ++++++++++--------- .../shared-ux/router/impl/routes_context.ts | 18 ++++++ packages/shared-ux/router/impl/services.ts | 8 ++- packages/shared-ux/router/impl/types.ts | 8 +++ .../router/impl/use_execution_context.ts | 2 +- .../public/application/index.tsx | 2 +- 15 files changed, 174 insertions(+), 49 deletions(-) create mode 100644 packages/shared-ux/router/impl/BUILD.bazel create mode 100644 packages/shared-ux/router/impl/routes_context.ts diff --git a/packages/react/kibana_context/render/render_provider.tsx b/packages/react/kibana_context/render/render_provider.tsx index a597311c0e17..345a0993bb9e 100644 --- a/packages/react/kibana_context/render/render_provider.tsx +++ b/packages/react/kibana_context/render/render_provider.tsx @@ -25,9 +25,13 @@ export type KibanaRenderContextProviderProps = Omit > = ({ children, ...props }) => { + const { analytics, executionContext, i18n, theme, userProfile, colorMode, modify } = props; return ( - - + + {children} diff --git a/packages/react/kibana_context/root/root_provider.test.tsx b/packages/react/kibana_context/root/root_provider.test.tsx index df4bdf14c5c2..90523ea24173 100644 --- a/packages/react/kibana_context/root/root_provider.test.tsx +++ b/packages/react/kibana_context/root/root_provider.test.tsx @@ -15,21 +15,26 @@ import { useEuiTheme } from '@elastic/eui'; import type { UseEuiTheme } from '@elastic/eui'; import { mountWithIntl } from '@kbn/test-jest-helpers'; import type { KibanaTheme } from '@kbn/react-kibana-context-common'; -import type { AnalyticsServiceStart } from '@kbn/core-analytics-browser'; -import { analyticsServiceMock } from '@kbn/core-analytics-browser-mocks'; +import type { ExecutionContextStart } from '@kbn/core-execution-context-browser'; +import { executionContextServiceMock } from '@kbn/core-execution-context-browser-mocks'; import { i18nServiceMock } from '@kbn/core-i18n-browser-mocks'; +import { I18nStart } from '@kbn/core-i18n-browser'; +import type { UserProfileService } from '@kbn/core-user-profile-browser'; +import { userProfileServiceMock } from '@kbn/core-user-profile-browser-mocks'; import { KibanaRootContextProvider } from './root_provider'; import { I18nStart } from '@kbn/core-i18n-browser'; describe('KibanaRootContextProvider', () => { let euiTheme: UseEuiTheme | undefined; let i18nMock: I18nStart; - let analytics: AnalyticsServiceStart; + let userProfile: UserProfileService; + let executionContext: ExecutionContextStart; beforeEach(() => { euiTheme = undefined; - analytics = analyticsServiceMock.createAnalyticsServiceStart(); i18nMock = i18nServiceMock.createStartContract(); + userProfile = userProfileServiceMock.createStart(); + executionContext = executionContextServiceMock.createStartContract(); }); const flushPromises = async () => { @@ -62,8 +67,9 @@ describe('KibanaRootContextProvider', () => { const wrapper = mountWithIntl( @@ -80,8 +86,9 @@ describe('KibanaRootContextProvider', () => { const wrapper = mountWithIntl( diff --git a/packages/react/kibana_context/root/root_provider.tsx b/packages/react/kibana_context/root/root_provider.tsx index f2c109fc6283..be8ddfa3f95a 100644 --- a/packages/react/kibana_context/root/root_provider.tsx +++ b/packages/react/kibana_context/root/root_provider.tsx @@ -11,6 +11,8 @@ import React, { FC, PropsWithChildren } from 'react'; import type { AnalyticsServiceStart } from '@kbn/core-analytics-browser'; import type { I18nStart } from '@kbn/core-i18n-browser'; +import type { ExecutionContextStart } from '@kbn/core-execution-context-browser'; +import { SharedUXRouterContext } from '@kbn/shared-ux-router'; // @ts-expect-error EUI exports this component internally, but Kibana isn't picking it up its types import { useIsNestedEuiProvider } from '@elastic/eui/lib/components/provider/nested'; @@ -25,6 +27,8 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps { i18n: I18nStart; /** The `AnalyticsServiceStart` API from `CoreStart`. */ analytics?: Pick; + /** The `ExecutionContextStart` API from `CoreStart`. */ + executionContext?: ExecutionContextStart; } /** @@ -44,19 +48,26 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps { export const KibanaRootContextProvider: FC> = ({ children, i18n, + executionContext, ...props }) => { const hasEuiProvider = useIsNestedEuiProvider(); + const rootContextProvider = ( + + {children} + + ); if (hasEuiProvider) { emitEuiProviderWarning( 'KibanaRootContextProvider has likely been nested in this React tree, either by direct reference or by KibanaRenderContextProvider. The result of this nesting is a nesting of EuiProvider, which has negative effects. Check your React tree for nested Kibana context providers.' ); - return {children}; + return rootContextProvider; } else { + const { theme, userProfile, globalStyles, colorMode, modify } = props; return ( - - {children} + + {rootContextProvider} ); } diff --git a/packages/react/kibana_context/root/tsconfig.json b/packages/react/kibana_context/root/tsconfig.json index 27ea0566f36a..8035f8379e39 100644 --- a/packages/react/kibana_context/root/tsconfig.json +++ b/packages/react/kibana_context/root/tsconfig.json @@ -22,6 +22,10 @@ "@kbn/core-i18n-browser", "@kbn/core-base-common", "@kbn/core-analytics-browser", - "@kbn/core-analytics-browser-mocks", + "@kbn/core-user-profile-browser", + "@kbn/core-user-profile-browser-mocks", + "@kbn/core-execution-context-browser", + "@kbn/core-execution-context-browser-mocks", + "@kbn/shared-ux-router" ] } diff --git a/packages/shared-ux/router/impl/BUILD.bazel b/packages/shared-ux/router/impl/BUILD.bazel new file mode 100644 index 000000000000..224bebcf72e4 --- /dev/null +++ b/packages/shared-ux/router/impl/BUILD.bazel @@ -0,0 +1,34 @@ +load("@build_bazel_rules_nodejs//:index.bzl", "js_library") + +SRCS = glob( + [ + "**/*.ts", + "**/*.tsx", + ], + exclude = [ + "**/test_helpers.ts", + "**/*.config.js", + "**/*.mock.*", + "**/*.test.*", + "**/*.stories.*", + "**/__snapshots__/**", + "**/integration_tests/**", + "**/mocks/**", + "**/scripts/**", + "**/storybook/**", + "**/test_fixtures/**", + "**/test_helpers/**", + ], +) + +DEPS = [ + +] + +js_library( + name = "shared-ux-router", + package_name = "@kbn/shared-ux-router", + srcs = ["package.json"] + SRCS, + deps = DEPS, + visibility = ["//visibility:public"], +) diff --git a/packages/shared-ux/router/impl/__snapshots__/route.test.tsx.snap b/packages/shared-ux/router/impl/__snapshots__/route.test.tsx.snap index 418aa60b7c1f..b7ef59528923 100644 --- a/packages/shared-ux/router/impl/__snapshots__/route.test.tsx.snap +++ b/packages/shared-ux/router/impl/__snapshots__/route.test.tsx.snap @@ -33,3 +33,5 @@ exports[`Route renders 1`] = ` `; + +exports[`Route renders with enableExecutionContextTracking as false 1`] = ``; diff --git a/packages/shared-ux/router/impl/index.ts b/packages/shared-ux/router/impl/index.ts index a8bef1b8499d..253d42beaa50 100644 --- a/packages/shared-ux/router/impl/index.ts +++ b/packages/shared-ux/router/impl/index.ts @@ -10,3 +10,5 @@ export { Route } from './route'; export { HashRouter, BrowserRouter, MemoryRouter, Router } from './router'; export { Routes } from './routes'; + +export { SharedUXRouterContext } from './services'; diff --git a/packages/shared-ux/router/impl/route.test.tsx b/packages/shared-ux/router/impl/route.test.tsx index 96bb6130387a..9d21690cdcf4 100644 --- a/packages/shared-ux/router/impl/route.test.tsx +++ b/packages/shared-ux/router/impl/route.test.tsx @@ -9,15 +9,34 @@ import React, { Component, FC } from 'react'; import { shallow } from 'enzyme'; +import { useSharedUXRoutesContext } from './routes_context'; import { Route } from './route'; import { createMemoryHistory } from 'history'; +jest.mock('./routes_context', () => ({ + useSharedUXRoutesContext: jest.fn().mockImplementation(() => ({ + enableExecutionContextTracking: true, + })), +})); + describe('Route', () => { + beforeEach(() => { + jest.restoreAllMocks(); + }); + test('renders', () => { const example = shallow(); expect(example).toMatchSnapshot(); }); + test('renders with enableExecutionContextTracking as false', () => { + (useSharedUXRoutesContext as jest.Mock).mockImplementationOnce(() => ({ + enableExecutionContextTracking: false, + })); + const example = shallow(); + expect(example).toMatchSnapshot(); + }); + test('location renders as expected', () => { // create a history const historyLocation = createMemoryHistory(); diff --git a/packages/shared-ux/router/impl/route.tsx b/packages/shared-ux/router/impl/route.tsx index 3535ff7aecec..5041f872b71b 100644 --- a/packages/shared-ux/router/impl/route.tsx +++ b/packages/shared-ux/router/impl/route.tsx @@ -15,6 +15,7 @@ import { RouteProps, useRouteMatch, } from 'react-router-dom'; +import { useSharedUXRoutesContext } from './routes_context'; import { useKibanaSharedUX } from './services'; import { useSharedUXExecutionContext } from './use_execution_context'; @@ -30,17 +31,18 @@ export const Route = ({ render, ...rest }: RouteProps) => { + const { enableExecutionContextTracking } = useSharedUXRoutesContext(); const component = useMemo(() => { if (!Component) { return undefined; } return (props: RouteComponentProps) => ( <> - + {enableExecutionContextTracking && } ); - }, [Component]); + }, [Component, enableExecutionContextTracking]); if (component) { return ; @@ -52,7 +54,7 @@ export const Route = ({ {...rest} render={(props) => ( <> - + {enableExecutionContextTracking && } {/* @ts-ignore else condition exists if renderFunction is undefined*/} {renderFunction(props)} @@ -62,7 +64,7 @@ export const Route = ({ } return ( - + {enableExecutionContextTracking && } {children} ); @@ -75,6 +77,12 @@ export const MatchPropagator = () => { const { executionContext } = useKibanaSharedUX().services; const match = useRouteMatch(); + if (!executionContext && process.env.NODE_ENV !== 'production') { + throw new Error( + 'Default execution context tracking is enabled but the executionContext service is not available' + ); + } + useSharedUXExecutionContext(executionContext, { type: 'application', page: match.path, diff --git a/packages/shared-ux/router/impl/routes.tsx b/packages/shared-ux/router/impl/routes.tsx index 9c1a97de6583..937756224627 100644 --- a/packages/shared-ux/router/impl/routes.tsx +++ b/packages/shared-ux/router/impl/routes.tsx @@ -13,6 +13,7 @@ import React, { Children } from 'react'; import { Switch, useRouteMatch } from 'react-router-dom'; import { Routes as ReactRouterRoutes, Route } from 'react-router-dom-v5-compat'; import { Route as LegacyRoute, MatchPropagator } from './route'; +import { SharedUXRoutesContext } from './routes_context'; type RouterElementChildren = Array< React.ReactElement< @@ -28,42 +29,47 @@ type RouterElementChildren = Array< export const Routes = ({ legacySwitch = true, + enableExecutionContextTracking = false, children, }: { legacySwitch?: boolean; + enableExecutionContextTracking?: boolean; children: React.ReactNode; }) => { const match = useRouteMatch(); return legacySwitch ? ( - {children} + + {children} + ) : ( - - {Children.map(children as RouterElementChildren, (child) => { - if (React.isValidElement(child) && child.type === LegacyRoute) { - const path = replace(child?.props.path, match.url + '/', ''); - const renderFunction = - typeof child?.props.children === 'function' - ? child?.props.children - : child?.props.render; - - return ( - - - {(child?.props?.component && ) || - (renderFunction && renderFunction()) || - children} - - } - /> - ); - } else { - return child; - } - })} - + + + {Children.map(children as RouterElementChildren, (child) => { + if (React.isValidElement(child) && child.type === LegacyRoute) { + const path = replace(child?.props.path, match.url + '/', ''); + const renderFunction = + typeof child?.props.children === 'function' + ? child?.props.children + : child?.props.render; + return ( + + {enableExecutionContextTracking && } + {(child?.props?.component && ) || + (renderFunction && renderFunction()) || + children} + + } + /> + ); + } else { + return child; + } + })} + + ); }; diff --git a/packages/shared-ux/router/impl/routes_context.ts b/packages/shared-ux/router/impl/routes_context.ts new file mode 100644 index 000000000000..115a5df7780d --- /dev/null +++ b/packages/shared-ux/router/impl/routes_context.ts @@ -0,0 +1,18 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { createContext, useContext } from 'react'; +import { SharedUXRoutesContextType } from './types'; + +const defaultContextValue = {}; + +export const SharedUXRoutesContext = createContext(defaultContextValue); + +export const useSharedUXRoutesContext = (): SharedUXRoutesContextType => + useContext(SharedUXRoutesContext as unknown as React.Context); diff --git a/packages/shared-ux/router/impl/services.ts b/packages/shared-ux/router/impl/services.ts index c3ad2ab06e06..7cea72f03bd8 100644 --- a/packages/shared-ux/router/impl/services.ts +++ b/packages/shared-ux/router/impl/services.ts @@ -50,7 +50,7 @@ export interface SharedUXExecutionContextSetup { */ export interface SharedUXExecutionContextSetup { /** {@link SharedUXExecutionContextSetup} */ - executionContext: SharedUXExecutionContextStart; + executionContext?: SharedUXExecutionContextStart; } export type KibanaServices = Partial; @@ -63,12 +63,14 @@ const defaultContextValue = { services: {}, }; -export const sharedUXContext = +export const SharedUXRouterContext = createContext>(defaultContextValue); export const useKibanaSharedUX = (): SharedUXRouterContextValue< KibanaServices & Extra > => useContext( - sharedUXContext as unknown as React.Context> + SharedUXRouterContext as unknown as React.Context< + SharedUXRouterContextValue + > ); diff --git a/packages/shared-ux/router/impl/types.ts b/packages/shared-ux/router/impl/types.ts index 833c5bdd0c81..067677d152ca 100644 --- a/packages/shared-ux/router/impl/types.ts +++ b/packages/shared-ux/router/impl/types.ts @@ -33,3 +33,11 @@ export declare interface SharedUXExecutionContext { /** an inner context spawned from the current context. */ child?: SharedUXExecutionContext; } + +export declare interface SharedUXRoutesContextType { + /** + * This flag is used to enable the default execution context tracking for a specific router. + * Enable this flag in case you don't have a custom implementation for execution context tracking. + * */ + readonly enableExecutionContextTracking?: boolean; +} diff --git a/packages/shared-ux/router/impl/use_execution_context.ts b/packages/shared-ux/router/impl/use_execution_context.ts index d460d4df3080..5e088fc3eac7 100644 --- a/packages/shared-ux/router/impl/use_execution_context.ts +++ b/packages/shared-ux/router/impl/use_execution_context.ts @@ -8,7 +8,7 @@ */ import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect'; -import { SharedUXExecutionContextSetup } from './services'; +import type { SharedUXExecutionContextSetup } from './services'; import { SharedUXExecutionContext } from './types'; /** diff --git a/x-pack/plugins/observability_solution/observability/public/application/index.tsx b/x-pack/plugins/observability_solution/observability/public/application/index.tsx index 3ae624d2f8ea..54b8b4044e64 100644 --- a/x-pack/plugins/observability_solution/observability/public/application/index.tsx +++ b/x-pack/plugins/observability_solution/observability/public/application/index.tsx @@ -28,7 +28,7 @@ import { HideableReactQueryDevTools } from './hideable_react_query_dev_tools'; function App() { return ( <> - + {Object.keys(routes).map((key) => { const path = key as keyof typeof routes; const { handler, exact } = routes[path]; From 2eb6c49d5a0e38d87202e73792e736af2f8de41d Mon Sep 17 00:00:00 2001 From: Maryam Saeidi Date: Thu, 19 Dec 2024 11:14:15 +0100 Subject: [PATCH 2/3] Remove profile dep --- packages/react/kibana_context/root/root_provider.test.tsx | 7 ------- packages/react/kibana_context/root/root_provider.tsx | 7 +------ packages/react/kibana_context/root/tsconfig.json | 2 -- 3 files changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/react/kibana_context/root/root_provider.test.tsx b/packages/react/kibana_context/root/root_provider.test.tsx index 90523ea24173..8752e03280d1 100644 --- a/packages/react/kibana_context/root/root_provider.test.tsx +++ b/packages/react/kibana_context/root/root_provider.test.tsx @@ -19,21 +19,16 @@ import type { ExecutionContextStart } from '@kbn/core-execution-context-browser' import { executionContextServiceMock } from '@kbn/core-execution-context-browser-mocks'; import { i18nServiceMock } from '@kbn/core-i18n-browser-mocks'; import { I18nStart } from '@kbn/core-i18n-browser'; -import type { UserProfileService } from '@kbn/core-user-profile-browser'; -import { userProfileServiceMock } from '@kbn/core-user-profile-browser-mocks'; import { KibanaRootContextProvider } from './root_provider'; -import { I18nStart } from '@kbn/core-i18n-browser'; describe('KibanaRootContextProvider', () => { let euiTheme: UseEuiTheme | undefined; let i18nMock: I18nStart; - let userProfile: UserProfileService; let executionContext: ExecutionContextStart; beforeEach(() => { euiTheme = undefined; i18nMock = i18nServiceMock.createStartContract(); - userProfile = userProfileServiceMock.createStart(); executionContext = executionContextServiceMock.createStartContract(); }); @@ -68,7 +63,6 @@ describe('KibanaRootContextProvider', () => { const wrapper = mountWithIntl( @@ -87,7 +81,6 @@ describe('KibanaRootContextProvider', () => { const wrapper = mountWithIntl( diff --git a/packages/react/kibana_context/root/root_provider.tsx b/packages/react/kibana_context/root/root_provider.tsx index be8ddfa3f95a..ff7374601746 100644 --- a/packages/react/kibana_context/root/root_provider.tsx +++ b/packages/react/kibana_context/root/root_provider.tsx @@ -64,11 +64,6 @@ export const KibanaRootContextProvider: FC - {rootContextProvider} - - ); + return {rootContextProvider}; } }; diff --git a/packages/react/kibana_context/root/tsconfig.json b/packages/react/kibana_context/root/tsconfig.json index 8035f8379e39..bde30f528140 100644 --- a/packages/react/kibana_context/root/tsconfig.json +++ b/packages/react/kibana_context/root/tsconfig.json @@ -22,8 +22,6 @@ "@kbn/core-i18n-browser", "@kbn/core-base-common", "@kbn/core-analytics-browser", - "@kbn/core-user-profile-browser", - "@kbn/core-user-profile-browser-mocks", "@kbn/core-execution-context-browser", "@kbn/core-execution-context-browser-mocks", "@kbn/shared-ux-router" From f0eb0c391a9f05ff4373411c179e866f7769c60e Mon Sep 17 00:00:00 2001 From: Maryam Saeidi Date: Thu, 19 Dec 2024 11:36:30 +0100 Subject: [PATCH 3/3] Revert changes to KibanaRenderContextProvider --- packages/react/kibana_context/render/render_provider.tsx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/react/kibana_context/render/render_provider.tsx b/packages/react/kibana_context/render/render_provider.tsx index 345a0993bb9e..a597311c0e17 100644 --- a/packages/react/kibana_context/render/render_provider.tsx +++ b/packages/react/kibana_context/render/render_provider.tsx @@ -25,13 +25,9 @@ export type KibanaRenderContextProviderProps = Omit > = ({ children, ...props }) => { - const { analytics, executionContext, i18n, theme, userProfile, colorMode, modify } = props; return ( - - + + {children}