Skip to content

Commit 1271edb

Browse files
kibanamachineDosantelasticmachine
authored
[9.2] [SideNav] Warnings and slight initial render improvement (#239721) (#240114)
# Backport This will backport the following commits from `main` to `9.2`: - [[SideNav] Warnings and slight initial render improvement (#239721)](#239721) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Anton Dosov","email":"anton.dosov@elastic.co"},"sourceCommit":{"committedDate":"2025-10-22T14:34:47Z","message":"[SideNav] Warnings and slight initial render improvement (#239721)\n\n## Summary\n\n- warnings fixes:\n - warn about duplicate ids in nav mapper \n - fix duplicate icons warning when icon is not a string\n - do not warn in prod\n- performance fixes: \n - mapper:\n- prevent unnecessary renders by separate navlinks update. we're not\nusing them directly, so removed\n- add debounceTime(0) to prevent unnecessary re-render where one\nobservable emits sync after another\n - ~nav (see videos before after)~\n - ~reduce re-renders when resizing~ \n- ~reduce height remeasures when items height isn't expected to change.\n**helps with initial render in security**~\n - moving to a separate pr\n\n\n~I think with this we can close\nhttps://github.com//issues/239331, because now tour in\nsecurity opens reliably and I don't see initial flash anymore~","sha":"1d87f15bbe1624b2c6cc1825d089f49e4dd8092d","branchLabelMapping":{"^v9.3.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:SharedUX","backport:version","v9.2.0","v9.3.0"],"title":"[SideNav] Warnings and slight initial render improvement","number":239721,"url":"https://github.com/elastic/kibana/pull/239721","mergeCommit":{"message":"[SideNav] Warnings and slight initial render improvement (#239721)\n\n## Summary\n\n- warnings fixes:\n - warn about duplicate ids in nav mapper \n - fix duplicate icons warning when icon is not a string\n - do not warn in prod\n- performance fixes: \n - mapper:\n- prevent unnecessary renders by separate navlinks update. we're not\nusing them directly, so removed\n- add debounceTime(0) to prevent unnecessary re-render where one\nobservable emits sync after another\n - ~nav (see videos before after)~\n - ~reduce re-renders when resizing~ \n- ~reduce height remeasures when items height isn't expected to change.\n**helps with initial render in security**~\n - moving to a separate pr\n\n\n~I think with this we can close\nhttps://github.com//issues/239331, because now tour in\nsecurity opens reliably and I don't see initial flash anymore~","sha":"1d87f15bbe1624b2c6cc1825d089f49e4dd8092d"}},"sourceBranch":"main","suggestedTargetBranches":["9.2"],"targetPullRequestStates":[{"branch":"9.2","label":"v9.2.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.3.0","branchLabelMappingKey":"^v9.3.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/239721","number":239721,"mergeCommit":{"message":"[SideNav] Warnings and slight initial render improvement (#239721)\n\n## Summary\n\n- warnings fixes:\n - warn about duplicate ids in nav mapper \n - fix duplicate icons warning when icon is not a string\n - do not warn in prod\n- performance fixes: \n - mapper:\n- prevent unnecessary renders by separate navlinks update. we're not\nusing them directly, so removed\n- add debounceTime(0) to prevent unnecessary re-render where one\nobservable emits sync after another\n - ~nav (see videos before after)~\n - ~reduce re-renders when resizing~ \n- ~reduce height remeasures when items height isn't expected to change.\n**helps with initial render in security**~\n - moving to a separate pr\n\n\n~I think with this we can close\nhttps://github.com//issues/239331, because now tour in\nsecurity opens reliably and I don't see initial flash anymore~","sha":"1d87f15bbe1624b2c6cc1825d089f49e4dd8092d"}}]}] BACKPORT--> Co-authored-by: Anton Dosov <anton.dosov@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
1 parent 59af2d5 commit 1271edb

4 files changed

Lines changed: 87 additions & 30 deletions

File tree

.github/CODEOWNERS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2949,6 +2949,7 @@ x-pack/solutions/observability/plugins/observability_shared/public/components/pr
29492949

29502950
# Shared UX
29512951
/src/core/packages/chrome @elastic/appex-sharedux
2952+
/src/core/packages/chrome/navigation @elastic/eui-team @elastic/appex-sharedux
29522953
/x-pack/platform/test/serverless/api_integration/test_suites/favorites @elastic/appex-sharedux # Assigned per https://github.com/elastic/kibana/pull/200985
29532954
/src/platform/test/api_integration/apis/short_url/**/*.ts @elastic/appex-sharedux # Assigned per https://github.com/elastic/kibana/pull/200209/files#r1846654156
29542955
/src/platform/test/api_integration/apis/unused_urls_task/**/*.ts @elastic/appex-sharedux # Assigned per https://github.com/elastic/kibana/pull/220138

src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/navigation/navigation.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,8 @@ const useNavigationItems = (
9696
props: Pick<ChromeNavigationProps, 'navigationTree$' | 'navLinks$' | 'activeNodes$' | 'basePath'>
9797
): NavigationItems | null => {
9898
const state$ = useMemo(
99-
() => combineLatest([props.navigationTree$, props.navLinks$, props.activeNodes$]),
100-
[props.navigationTree$, props.navLinks$, props.activeNodes$]
99+
() => combineLatest([props.navigationTree$, props.activeNodes$]),
100+
[props.navigationTree$, props.activeNodes$]
101101
);
102102
const state = useObservable(state$);
103103

@@ -106,8 +106,8 @@ const useNavigationItems = (
106106

107107
const memoizedItems = useMemo(() => {
108108
if (!state) return null;
109-
const [navigationTree, navLinks, activeNodes] = state;
110-
return toNavigationItems(navigationTree, navLinks, activeNodes, panelStateManager);
109+
const [navigationTree, activeNodes] = state;
110+
return toNavigationItems(navigationTree, activeNodes, panelStateManager);
111111
}, [state, panelStateManager]);
112112

113113
return memoizedItems;

src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/navigation/to_navigation_items.test.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const createNavigationItems = (
2929
tree: NavigationTreeDefinitionUI = navigationTree,
3030
activeNodes: ChromeProjectNavigationNode[][] = []
3131
) => {
32-
return toNavigationItems(tree, [], activeNodes, mockPanelStateManager);
32+
return toNavigationItems(tree, activeNodes, mockPanelStateManager);
3333
};
3434

3535
beforeEach(() => {
@@ -96,7 +96,8 @@ describe('toNavigationItems', () => {
9696
• No icon found for node \\"securityGroup:assets\\". Expected iconV2, icon, deepLink.euiIconType, deepLink.icon or a known deep link id. Using fallback icon \\"broom\\".
9797
• No icon found for node \\"securityGroup:machineLearning\\". Expected iconV2, icon, deepLink.euiIconType, deepLink.icon or a known deep link id. Using fallback icon \\"broom\\".
9898
• No icon found for node \\"stack_management\\". Expected iconV2, icon, deepLink.euiIconType, deepLink.icon or a known deep link id. Using fallback icon \\"broom\\".
99-
• Accordion items are not supported in the new navigation. Flattening them \\"stack_management, monitoring, integrations\\" and dropping accordion node \\"node-2\\"."
99+
• Accordion items are not supported in the new navigation. Flattening them \\"stack_management, monitoring, integrations\\" and dropping accordion node \\"node-2\\".
100+
• ID \\"endpoints\\" is used 2 times in navigation items. Each navigation item must have a unique ID."
100101
`);
101102
});
102103
});

src/core/packages/chrome/browser-internal/src/ui/project/sidenav_v2/navigation/to_navigation_items.tsx

Lines changed: 79 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
*/
99

1010
import type {
11-
ChromeNavLink,
1211
ChromeProjectNavigationNode,
1312
NavigationTreeDefinitionUI,
1413
RecentlyAccessedDefinition,
@@ -28,6 +27,8 @@ import { isActiveFromUrl } from '@kbn/shared-ux-chrome-navigation/src/utils';
2827
import { AppDeepLinkIdToIcon } from './known_icons_mappings';
2928
import type { PanelStateManager } from './panel_state_manager';
3029

30+
const SKIP_WARNINGS = process.env.NODE_ENV === 'production';
31+
3132
export interface NavigationItems {
3233
logoItem: SideNavLogo;
3334
navItems: NavigationStructure;
@@ -59,9 +60,6 @@ export interface NavigationItems {
5960
*/
6061
export const toNavigationItems = (
6162
navigationTree: NavigationTreeDefinitionUI,
62-
// navLinks and activeNodes are not used yet, but are passed for future use
63-
// they will be needed for isActive state management and possibly for rendering links
64-
navLinks: Readonly<ChromeNavLink[]>,
6563
activeNodes: ChromeProjectNavigationNode[][],
6664
panelStateManager: PanelStateManager
6765
): NavigationItems => {
@@ -322,8 +320,10 @@ export const toNavigationItems = (
322320
);
323321
}
324322

325-
// Check for duplicate icons
326-
warnAboutDuplicateIcons(logoItem, primaryItems);
323+
if (!SKIP_WARNINGS) {
324+
warnAboutDuplicateIds(logoItem, primaryItems, footerItems);
325+
warnAboutDuplicateIcons(logoItem, primaryItems, footerItems);
326+
}
327327

328328
return {
329329
logoItem,
@@ -376,6 +376,7 @@ function warnIfMissing<T extends { id: string }, K extends keyof T>(
376376
const warnedMessages = new Set<string>();
377377
let lastWarning = '';
378378
function warnOnce(message: string) {
379+
if (SKIP_WARNINGS) return;
379380
if (!warnedMessages.has(message)) {
380381
warnedMessages.add(message);
381382
}
@@ -429,30 +430,84 @@ const isRecentlyAccessedDefinition = (
429430
const filterEmpty = <T,>(arr: Array<T | null | undefined>): T[] =>
430431
arr.filter((item) => item !== null && item !== undefined) as T[];
431432

432-
function warnAboutDuplicateIcons(logoItem: SideNavLogo, primaryItems: MenuItem[]) {
433-
// Collect all items with icons
434-
const itemsWithIcons = [logoItem, ...primaryItems]
435-
.filter((item) => item.iconType && item.iconType !== FALLBACK_ICON)
436-
.map((item) => ({ id: item.id, icon: String(item.iconType) }));
437-
438-
// Group items by icon and warn about duplicates
439-
const iconGroups = new Map<string, string[]>();
440-
itemsWithIcons.forEach(({ id, icon }) => {
441-
const items = iconGroups.get(icon) || [];
442-
iconGroups.set(icon, [...items, id]);
433+
/**
434+
* Generic function to detect and warn about duplicate values in navigation items.
435+
* @param values - Array of values to check for duplicates
436+
* @param formatWarning - Function to format the warning message for duplicates
437+
*/
438+
function warnAboutDuplicates(
439+
values: string[],
440+
formatWarning: (value: string, count: number) => string
441+
) {
442+
const valueGroups = new Map<string, number>();
443+
values.forEach((value) => {
444+
valueGroups.set(value, (valueGroups.get(value) || 0) + 1);
443445
});
444446

445-
iconGroups.forEach((itemIds, iconType) => {
446-
if (itemIds.length > 1) {
447-
warnOnce(
448-
`Icon "${iconType}" is used by multiple navigation items: ${itemIds.join(
449-
', '
450-
)}. Consider using unique icons for better UX.`
451-
);
447+
valueGroups.forEach((count, value) => {
448+
if (count > 1) {
449+
warnOnce(formatWarning(value, count));
452450
}
453451
});
454452
}
455453

454+
function warnAboutDuplicateIcons(
455+
logoItem: SideNavLogo,
456+
primaryItems: MenuItem[],
457+
footerItems: MenuItem[]
458+
) {
459+
if (SKIP_WARNINGS) return;
460+
// Collect all items with icons (only logo + primary items, excluding fallback)
461+
const icons = [logoItem, ...primaryItems, ...footerItems]
462+
.filter(
463+
(item) =>
464+
item.iconType && item.iconType !== FALLBACK_ICON && typeof item.iconType === 'string'
465+
)
466+
.map((item) => String(item.iconType));
467+
468+
warnAboutDuplicates(
469+
icons,
470+
(icon) =>
471+
`Icon "${icon}" is used by multiple navigation items. Consider using unique icons for better UX.`
472+
);
473+
}
474+
475+
function warnAboutDuplicateIds(
476+
logoItem: SideNavLogo,
477+
primaryItems: MenuItem[],
478+
footerItems: MenuItem[]
479+
) {
480+
if (SKIP_WARNINGS) return;
481+
// Collect all IDs from all items, including secondary menu items
482+
let allIds: string[] = [logoItem.id];
483+
484+
// Helper to extract IDs from menu items including their secondary sections
485+
const collectIds = (items: MenuItem[]) => {
486+
items.forEach((item) => {
487+
allIds.push(item.id);
488+
if (item.sections) {
489+
item.sections.forEach((section) => {
490+
allIds.push(section.id);
491+
section.items.forEach((secondaryItem) => {
492+
allIds.push(secondaryItem.id);
493+
});
494+
});
495+
}
496+
});
497+
};
498+
499+
collectIds(primaryItems);
500+
collectIds(footerItems);
501+
502+
allIds = allIds.filter((id) => !id?.startsWith('node-')); // Filter out auto-generated IDs
503+
504+
warnAboutDuplicates(
505+
allIds,
506+
(id, count) =>
507+
`ID "${id}" is used ${count} times in navigation items. Each navigation item must have a unique ID.`
508+
);
509+
}
510+
456511
const FALLBACK_ICON = 'broom' as const;
457512
/**
458513
* Finds an item href based on the last active item history for a panel opener.

0 commit comments

Comments
 (0)