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

Fix context.pageName by fixing missing executionContext #201118

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions packages/react/kibana_context/root/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ DEPS = [
"@npm//tslib",
"@npm//@elastic/eui",
"//packages/core/base/core-base-common",
"//packages/shared-ux/router/impl:shared-ux-router",
]

js_library(
Expand Down
6 changes: 6 additions & 0 deletions packages/react/kibana_context/root/root_provider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ 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 { KibanaRootContextProvider } from './root_provider';
import { I18nStart } from '@kbn/core-i18n-browser';
Expand All @@ -25,11 +27,13 @@ describe('KibanaRootContextProvider', () => {
let euiTheme: UseEuiTheme | undefined;
let i18nMock: I18nStart;
let analytics: AnalyticsServiceStart;
let executionContext: ExecutionContextStart;

beforeEach(() => {
euiTheme = undefined;
analytics = analyticsServiceMock.createAnalyticsServiceStart();
i18nMock = i18nServiceMock.createStartContract();
executionContext = executionContextServiceMock.createStartContract();
});

const flushPromises = async () => {
Expand Down Expand Up @@ -64,6 +68,7 @@ describe('KibanaRootContextProvider', () => {
<KibanaRootContextProvider
analytics={analytics}
i18n={i18nMock}
executionContext={executionContext}
theme={{ theme$: of(coreTheme) }}
>
<InnerComponent />
Expand All @@ -82,6 +87,7 @@ describe('KibanaRootContextProvider', () => {
<KibanaRootContextProvider
analytics={analytics}
i18n={i18nMock}
executionContext={executionContext}
theme={{ theme$: coreTheme$ }}
>
<InnerComponent />
Expand Down
18 changes: 12 additions & 6 deletions packages/react/kibana_context/root/root_provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 { SharedUXContext } 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';
Expand All @@ -25,6 +27,8 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps {
i18n: I18nStart;
/** The `AnalyticsServiceStart` API from `CoreStart`. */
analytics?: Pick<AnalyticsServiceStart, 'reportEvent'>;
/** The `ExecutionContextStart` API from `CoreStart`. */
executionContext?: ExecutionContextStart;
Comment on lines +30 to +31
Copy link
Member

Choose a reason for hiding this comment

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

Q: to avoid errors in the future, should we make this one mandatory?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check to see if it is OK to make this dependency mandatory.
Still, I don't think this will prevent similar issues in the future because, eventually, it will be passed to SharedUXContext, which is initialized with an empty executionContext here.

}

/**
Expand All @@ -44,20 +48,22 @@ export interface KibanaRootContextProviderProps extends KibanaEuiProviderProps {
export const KibanaRootContextProvider: FC<PropsWithChildren<KibanaRootContextProviderProps>> = ({
children,
i18n,
executionContext,
...props
}) => {
const hasEuiProvider = useIsNestedEuiProvider();
const rootContextProvider = (
<SharedUXContext.Provider value={{ services: { executionContext } }}>
<i18n.Context>{children}</i18n.Context>
</SharedUXContext.Provider>
);

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 <i18n.Context>{children}</i18n.Context>;
return rootContextProvider;
} else {
return (
<KibanaEuiProvider {...props}>
<i18n.Context>{children}</i18n.Context>
</KibanaEuiProvider>
);
return <KibanaEuiProvider {...props}>{rootContextProvider}</KibanaEuiProvider>;
}
};
3 changes: 3 additions & 0 deletions packages/react/kibana_context/root/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,8 @@
"@kbn/core-base-common",
"@kbn/core-analytics-browser",
"@kbn/core-analytics-browser-mocks",
"@kbn/core-execution-context-browser",
"@kbn/core-execution-context-browser-mocks",
"@kbn/shared-ux-router",
]
}
34 changes: 34 additions & 0 deletions packages/shared-ux/router/impl/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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"],
)
2 changes: 2 additions & 0 deletions packages/shared-ux/router/impl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@
export { Route } from './route';
export { HashRouter, BrowserRouter, MemoryRouter, Router } from './router';
export { Routes } from './routes';

export { SharedUXContext } from './services';
6 changes: 3 additions & 3 deletions packages/shared-ux/router/impl/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export interface SharedUXExecutionContextSetup {
*/
export interface SharedUXExecutionContextSetup {
/** {@link SharedUXExecutionContextSetup} */
executionContext: SharedUXExecutionContextStart;
executionContext?: SharedUXExecutionContextStart;
}

export type KibanaServices = Partial<SharedUXExecutionContextSetup>;
Expand All @@ -63,12 +63,12 @@ const defaultContextValue = {
services: {},
};

export const sharedUXContext =
export const SharedUXContext =
createContext<SharedUXRouterContextValue<KibanaServices>>(defaultContextValue);

export const useKibanaSharedUX = <Extra extends object = {}>(): SharedUXRouterContextValue<
KibanaServices & Extra
> =>
useContext(
sharedUXContext as unknown as React.Context<SharedUXRouterContextValue<KibanaServices & Extra>>
SharedUXContext as unknown as React.Context<SharedUXRouterContextValue<KibanaServices & Extra>>
);
2 changes: 1 addition & 1 deletion packages/shared-ux/router/impl/use_execution_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import useDeepCompareEffect from 'react-use/lib/useDeepCompareEffect';
import { SharedUXExecutionContextSetup } from './services';
import type { SharedUXExecutionContextSetup } from './services';
import { SharedUXExecutionContext } from './types';

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const DashboardListingTable = ({
urlStateEnabled,
showCreateDashboardButton = true,
}: DashboardListingProps) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

dashboard changes LGTM, comment needs to be removed

useExecutionContext(coreServices.executionContext, {
type: 'application',
page: 'list',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export const ContextApp = ({ dataView, anchorId, referrer }: ContextAppProps) =>
});
}, [locator, referrer, services]);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I reviewed our usages in Discover and dropping the custom overrides changes the context properties. E.g. for this one specifically:

page: "context" -> page: "/context/:dataViewId/:id"
entityId: "90943e30-9a47-11e8-b64d-95841ca0b247" -> entityId: "{\"dataViewId\":\"90943e30-9a47-11e8-b64d-95841ca0b247\",\"id\":\"Am8GZZMBvuc8nvvEfgGJ\"}"

I kept the overrides and removed the TODO here: 89b004b.

useExecutionContext(core.executionContext, {
type: 'application',
page: 'context',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export const VisualizeEditor = ({ onAppLeave }: VisualizeAppProps) => {
);

const editorName = savedVisInstance?.vis.type.title.toLowerCase().replace(' ', '_') || '';
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I've checked this and noticed some subtle different pageName sometimes.
Nothing major, just these few issues:

  • application:visualize and application:visualize:editor are used for similar flows
  • application:visualize:editor:xxxxx is triggered only on navigating back to dashboard from editor, not on enter

useExecutionContext(services.executionContext, {
type: 'application',
page: `editor${editorName ? `:${editorName}` : ''}`,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/aiops/public/hooks/use_data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const useData = (
) => {
const { executionContext, uiSettings } = useAiopsAppContext();

// TODO: Remove this comment after testing
useExecutionContext(executionContext, {
name: AIOPS_PLUGIN_ID,
type: 'application',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const AppWithExecutionContext = ({
getUrlForApp: ApplicationStart['getUrlForApp'];
executionContext: ExecutionContextStart;
}) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a difference here:

Screenshot 2024-11-28 at 13 19 31

useExecutionContext(executionContext, {
type: 'application',
page: 'crossClusterReplication',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/data_quality/public/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const AppWithExecutionContext = ({
}) => {
const { executionContext } = core;

// TODO: Remove this comment after testing
useExecutionContext(executionContext, {
type: 'application',
page: PLUGIN_ID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export const useData = (
},
} = useDataVisualizerKibana();

// TODO: Remove this comment after testing
useExecutionContext(executionContext, {
name: 'data_visualizer',
type: 'application',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const IndexList: React.FunctionComponent<RouteComponentProps> = ({ histor
core: { executionContext },
} = useAppContext();

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

useExecutionContext(executionContext, {
type: 'application',
page: 'indexManagementIndicesTab',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/lens/public/app_plugin/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ export function App({
setIndicateNoData(true);
}, [setIndicateNoData]);

// TODO: Remove this comment after testing
Copy link
Contributor

@dej611 dej611 Nov 29, 2024

Choose a reason for hiding this comment

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

Not sure it is strictly related to this PR, but from my quick testing I see these page names from different flows:

  • Create visualization in dashboard, opens a Lens editor => application:lens:editor βœ…
  • Create ES|QL panel in dashboard => application:dashboards:app ❓
  • Edit inline in a Dashboard => application:dashboard:app ❓
  • Edit in editor from a Dashboard inline editing panel => application:lens:editor βœ…
  • Edit visualization in Discover => application:lens:/edit_by_value ❓

useExecutionContext(executionContext, {
type: 'application',
id: savedObjectId || 'new', // TODO: this doesn't consider when lens is saved by value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export const useActiveRoute = (routesList: MlRoute[]): MlRoute => {
[activeRoute, overlays, pathname, startServices]
);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested, works as expected

Suggested change
// TODO: Remove this comment after testing

useExecutionContext(executionContext, {
name: PLUGIN_ID,
type: 'application',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/rollup/public/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const AppWithExecutionContext = ({
history: ManagementAppMountParams['history'];
executionContext: ExecutionContextStart;
}) => {
// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a difference here:

Screenshot 2024-11-28 at 13 23 38

useExecutionContext(executionContext, {
type: 'application',
page: 'rollup',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const useUpdateExecutionContext = () => {
useEffect(() => {
// setImmediate is required to ensure that EBT telemetry for the previous page is shipped before the new page is updated
setImmediate(() => {
// TODO: Remove this comment after testing
executionContext.set({ page: pathname });
Copy link
Member Author

@maryam-saeidi maryam-saeidi Nov 25, 2024

Choose a reason for hiding this comment

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

@kpatticha noticed a difference in the new context:

image

@elastic/security-solution Any idea how we can solve this issue?
In APM customized execution context, overriding the value is working as expected with the changes in this PR. I wonder what the difference between these two cases is.

Copy link
Contributor

@Dosant Dosant Nov 28, 2024

Choose a reason for hiding this comment

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

It seems like the same problem as with the data_quality page. The custom hook is called before the hook in our Route

Copy link
Contributor

@Dosant Dosant Nov 28, 2024

Choose a reason for hiding this comment

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

And there is slight implementation difference which is probably why the pageName is different

we use : const match = useRouteMatch(); match.path
security uses: const { pathname } = useLocation();

});
}, [pathname, executionContext]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export const PolicyList: React.FunctionComponent<RouteComponentProps<MatchParams
uiMetricService.trackUiMetric(UIM_POLICY_LIST_LOAD);
}, [uiMetricService]);

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

useExecutionContext(core.executionContext, {
type: 'application',
page: 'snapshotRestorePolicyTab',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export const RootComponent = (dependencies: AppDependencies) => {
core: { application, http, executionContext, ...startServices },
} = dependencies.services;

// TODO: Remove this comment after testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Lgtm, removed comment with 2a5a780.

executionContext.set({ type: 'application', page: 'upgradeAssistant' });

return (
Expand Down