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.

Sign up for GitHub

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
Open

Fix context.pageName by fixing missing executionContext #201118

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3925c21
Fix missing executionContext
maryam-saeidi Nov 21, 2024
d0b0d51
Revert ExecutionContextStart directly
maryam-saeidi Nov 21, 2024
b0c621c
add router to bazel
Dosant Nov 21, 2024
dcc81b8
Merge pull request #4 from Dosant/195778-fix-pageName
maryam-saeidi Nov 21, 2024
68cae60
Add temporary TODOs
maryam-saeidi Nov 25, 2024
3f82e98
Merge branch 'main' into 195778-fix-pageName
maryam-saeidi Nov 25, 2024
87ae813
Revert useExecutionContext test in APM
maryam-saeidi Nov 25, 2024
89b004b
Remove TODO from src/plugins/discover/public/application/context/cont…
davismcphee Nov 25, 2024
81a712c
fix(dataset-quality): remove TODO
Nov 26, 2024
7e4df5d
Merge branch 'main' into 195778-fix-pageName
maryam-saeidi Nov 26, 2024
2a5a780
Remove comments for IM, Snapshot restore, UA
ElenaStoeva Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/react/kibana_context/root/BUILD.bazel
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions src/plugins/dashboard/public/dashboard_listing/dashboard_listing_table.tsx
View file
Open in desktop
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
1 change: 1 addition & 0 deletions src/plugins/visualizations/public/visualize_app/components/visualize_editor.tsx
View file
Open in desktop
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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/plugins/cross_cluster_replication/public/app/index.tsx
View file
Open in desktop
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_visualizer/public/application/common/hooks/use_data.ts
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/plugins/index_management/public/application/sections/home/index_list/index_list.tsx
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ export function App({
setIndicateNoData(true);
}, [setIndicateNoData]);

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

@dej611 dej611 Nov 29, 2024 •
edited
Loading

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
1 change: 1 addition & 0 deletions x-pack/plugins/ml/public/application/routing/use_active_route.tsx
View file
Open in desktop
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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/public/common/hooks/use_update_execution_context.ts
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
1 change: 1 addition & 0 deletions x-pack/plugins/snapshot_restore/public/application/sections/home/policy_list/policy_list.tsx
View file
Open in desktop
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
1 change: 1 addition & 0 deletions x-pack/plugins/upgrade_assistant/public/application/app.tsx
View file
Open in desktop
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