From 6dd711b6f2d78684bb5e87da61f879a2861c6ef0 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Tue, 16 Dec 2025 17:52:08 +0100 Subject: [PATCH] Scopes: Fix selector crashing on unavailable node (#115417) * Fix crash * Add regression tests * Remove component mocking --- .../scopes/selector/ScopesTree.test.tsx | 248 ++++++++++++++++++ .../features/scopes/selector/ScopesTree.tsx | 29 +- .../scopes/selector/ScopesTreeHeadline.tsx | 2 +- .../selector/ScopesTreeItemList.test.tsx | 136 ++++++++++ .../scopes/selector/ScopesTreeItemList.tsx | 9 +- 5 files changed, 407 insertions(+), 17 deletions(-) create mode 100644 public/app/features/scopes/selector/ScopesTree.test.tsx create mode 100644 public/app/features/scopes/selector/ScopesTreeItemList.test.tsx diff --git a/public/app/features/scopes/selector/ScopesTree.test.tsx b/public/app/features/scopes/selector/ScopesTree.test.tsx new file mode 100644 index 00000000000..2ac4b647a5c --- /dev/null +++ b/public/app/features/scopes/selector/ScopesTree.test.tsx @@ -0,0 +1,248 @@ +import { render, screen } from '@testing-library/react'; + +import { ScopeNode } from '@grafana/data'; + +import { ScopesTree } from './ScopesTree'; +import { NodesMap, SelectedScope, TreeNode } from './types'; + +// Mock the ScopesContextProvider hook since it requires a full context setup +jest.mock('../ScopesContextProvider', () => ({ + useScopesServices: () => ({ + scopesSelectorService: { + closeAndApply: jest.fn(), + }, + }), +})); + +describe('ScopesTree', () => { + const mockFilterNode = jest.fn(); + const mockSelectScope = jest.fn(); + const mockDeselectScope = jest.fn(); + const mockToggleExpandedNode = jest.fn(); + + const createMockScopeNode = (name: string, parentName?: string): ScopeNode => ({ + metadata: { name }, + spec: { + title: `Title ${name}`, + nodeType: 'leaf', + linkType: 'scope', + linkId: `scope-${name}`, + parentName: parentName ?? '', + }, + }); + + const defaultScopeNodes: NodesMap = { + 'parent-container': { + metadata: { name: 'parent-container' }, + spec: { + title: 'Parent Container', + nodeType: 'container', + parentName: '', + }, + }, + 'child-1': createMockScopeNode('child-1', 'parent-container'), + 'child-2': createMockScopeNode('child-2', 'parent-container'), + }; + + const defaultTree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: { + 'child-1': { scopeNodeId: 'child-1', expanded: false, query: '' }, + 'child-2': { scopeNodeId: 'child-2', expanded: false, query: '' }, + }, + childrenLoaded: true, + }; + + const defaultProps = { + tree: defaultTree, + loadingNodeName: undefined, + selectedScopes: [] as SelectedScope[], + scopeNodes: defaultScopeNodes, + filterNode: mockFilterNode, + selectScope: mockSelectScope, + deselectScope: mockDeselectScope, + toggleExpandedNode: mockToggleExpandedNode, + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('selectedNodesToShow logic', () => { + it('should not show selectedNodesToShow when no scopes are selected', () => { + render(); + + // Both child-1 and child-2 should be visible in the regular children list + expect(screen.getByText('Title child-1')).toBeInTheDocument(); + expect(screen.getByText('Title child-2')).toBeInTheDocument(); + }); + + it('should only consider first selected scope for selectedNodesToShow', () => { + const selectedScopes: SelectedScope[] = [ + { scopeId: 'scope-1', scopeNodeId: 'child-1' }, + { scopeId: 'scope-2', scopeNodeId: 'child-2' }, + ]; + + // Use a tree where child-1 is NOT in the children (to trigger selectedNodesToShow) + const tree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: { + // child-1 is NOT here, so it should appear in selectedNodesToShow + 'child-2': { scopeNodeId: 'child-2', expanded: false, query: '' }, + }, + childrenLoaded: true, + }; + + render(); + + // child-1 should be shown (from selectedNodesToShow - only first scope is considered) + expect(screen.getByText('Title child-1')).toBeInTheDocument(); + // child-2 should also be shown (from regular children) + expect(screen.getByText('Title child-2')).toBeInTheDocument(); + }); + + it('should not show selectedNodesToShow when first scope has no scopeNodeId', () => { + const selectedScopes: SelectedScope[] = [ + { scopeId: 'scope-1', scopeNodeId: undefined }, // No scopeNodeId + { scopeId: 'scope-2', scopeNodeId: 'child-2' }, + ]; + + // Tree with no children to make it obvious if selectedNodesToShow is populated + const tree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: {}, + childrenLoaded: true, + }; + + render(); + + // child-2 should NOT appear because only first scope is considered and it has no scopeNodeId + expect(screen.queryByText('Title child-2')).not.toBeInTheDocument(); + }); + + it('should not show selectedNodesToShow when first scope node is not in scopeNodes cache', () => { + const selectedScopes: SelectedScope[] = [ + { scopeId: 'scope-1', scopeNodeId: 'missing-node' }, // Node not in scopeNodes + { scopeId: 'scope-2', scopeNodeId: 'child-2' }, + ]; + + // Tree with no children + const tree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: {}, + childrenLoaded: true, + }; + + render(); + + // Neither should appear since first scope's node is missing from cache + expect(screen.queryByText('Title missing-node')).not.toBeInTheDocument(); + expect(screen.queryByText('Title child-2')).not.toBeInTheDocument(); + }); + + it('should not show selectedNodesToShow when tree scopeNodeId does not match first scope parent', () => { + const selectedScopes: SelectedScope[] = [ + { scopeId: 'scope-1', scopeNodeId: 'child-1' }, // child-1's parent is 'parent-container' + ]; + + // Tree with different scopeNodeId + const tree: TreeNode = { + scopeNodeId: 'different-container', // Different from child-1's parent + expanded: true, + query: '', + children: {}, + childrenLoaded: true, + }; + + const scopeNodes: NodesMap = { + ...defaultScopeNodes, + 'different-container': { + metadata: { name: 'different-container' }, + spec: { title: 'Different', nodeType: 'container', parentName: '' }, + }, + }; + + render(); + + // child-1 should NOT appear since tree's scopeNodeId doesn't match child-1's parent + expect(screen.queryByText('Title child-1')).not.toBeInTheDocument(); + }); + + it('should not duplicate scope in selectedNodesToShow if already in children', () => { + const selectedScopes: SelectedScope[] = [{ scopeId: 'scope-1', scopeNodeId: 'child-1' }]; + + // Tree already has child-1 in children + const tree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: { + 'child-1': { scopeNodeId: 'child-1', expanded: false, query: '' }, + 'child-2': { scopeNodeId: 'child-2', expanded: false, query: '' }, + }, + childrenLoaded: true, + }; + + render(); + + // child-1 should appear exactly once (in regular children, not duplicated) + const child1Elements = screen.getAllByText('Title child-1'); + expect(child1Elements).toHaveLength(1); + }); + }); + + describe('graceful handling of missing data', () => { + it('should not crash when scopeNodes is empty', () => { + const tree: TreeNode = { + scopeNodeId: '', + expanded: true, + query: '', + children: {}, + childrenLoaded: true, + }; + + render(); + + // Should render without crashing - search input should be present + expect(screen.getByRole('combobox')).toBeInTheDocument(); + }); + + it('should handle tree with children referencing missing nodes', () => { + const tree: TreeNode = { + scopeNodeId: 'parent-container', + expanded: true, + query: '', + children: { + 'existing-node': { scopeNodeId: 'existing-node', expanded: false, query: '' }, + 'missing-node': { scopeNodeId: 'missing-node', expanded: false, query: '' }, + }, + childrenLoaded: true, + }; + + const scopeNodes: NodesMap = { + 'parent-container': { + metadata: { name: 'parent-container' }, + spec: { title: 'Parent', nodeType: 'container', parentName: '' }, + }, + 'existing-node': createMockScopeNode('existing-node', 'parent-container'), + // 'missing-node' intentionally not included + }; + + // Should render without crashing + render(); + + // Existing node should be rendered + expect(screen.getByText('Title existing-node')).toBeInTheDocument(); + // Missing node should be gracefully skipped + expect(screen.queryByText('Title missing-node')).not.toBeInTheDocument(); + }); + }); +}); diff --git a/public/app/features/scopes/selector/ScopesTree.tsx b/public/app/features/scopes/selector/ScopesTree.tsx index dd2fd61184c..8178acd3368 100644 --- a/public/app/features/scopes/selector/ScopesTree.tsx +++ b/public/app/features/scopes/selector/ScopesTree.tsx @@ -55,21 +55,22 @@ export function ScopesTree({ const anyChildExpanded = childrenArray.some(({ expanded }) => expanded); // Nodes that are already selected (not applied) are always shown if we are in their category, even if they are - // filtered out by query filter + // filtered out by query filter. Only consider the first selected scope for this display logic. let selectedNodesToShow: TreeNode[] = []; - if (selectedScopes.length > 0 && selectedScopes[0].scopeNodeId) { - if (tree.scopeNodeId === scopeNodes[selectedScopes[0].scopeNodeId]?.spec.parentName) { - selectedNodesToShow = selectedScopes - // We filter out those which are still shown in the normal list of results - .filter((s) => !childrenArray.map((c) => c.scopeNodeId).includes(s.scopeNodeId!)) - .map((s) => ({ - // Because we had to check the parent with the use of scopeNodeId we know we have it. (we may not have it - // if the selected scopes are from url persistence, in which case we don't show them) - scopeNodeId: s.scopeNodeId!, - query: '', - expanded: false, - })); - } + const firstSelectedScope = selectedScopes[0]; + if ( + firstSelectedScope?.scopeNodeId && + scopeNodes[firstSelectedScope.scopeNodeId] && + tree.scopeNodeId === scopeNodes[firstSelectedScope.scopeNodeId]?.spec.parentName && + !childrenArray.map((c) => c.scopeNodeId).includes(firstSelectedScope.scopeNodeId) + ) { + selectedNodesToShow = [ + { + scopeNodeId: firstSelectedScope.scopeNodeId, + query: '', + expanded: false, + }, + ]; } const { highlightedId, ariaActiveDescendant, enableHighlighting, disableHighlighting } = useScopesHighlighting({ diff --git a/public/app/features/scopes/selector/ScopesTreeHeadline.tsx b/public/app/features/scopes/selector/ScopesTreeHeadline.tsx index 755fadf64b5..44ed80ce1bf 100644 --- a/public/app/features/scopes/selector/ScopesTreeHeadline.tsx +++ b/public/app/features/scopes/selector/ScopesTreeHeadline.tsx @@ -18,7 +18,7 @@ export function ScopesTreeHeadline({ anyChildExpanded, query, resultsNodes, scop if ( anyChildExpanded || - (resultsNodes.some((n) => scopeNodes[n.scopeNodeId].spec.nodeType === 'container') && !query) + (resultsNodes.some((n) => scopeNodes[n.scopeNodeId]?.spec.nodeType === 'container') && !query) ) { return null; } diff --git a/public/app/features/scopes/selector/ScopesTreeItemList.test.tsx b/public/app/features/scopes/selector/ScopesTreeItemList.test.tsx new file mode 100644 index 00000000000..a93bb456a83 --- /dev/null +++ b/public/app/features/scopes/selector/ScopesTreeItemList.test.tsx @@ -0,0 +1,136 @@ +import { render, screen } from '@testing-library/react'; + +import { ScopeNode } from '@grafana/data'; + +import { ScopesTreeItemList } from './ScopesTreeItemList'; +import { NodesMap, SelectedScope, TreeNode } from './types'; + +// Mock the ScopesContextProvider hook since it requires a full context setup +jest.mock('../ScopesContextProvider', () => ({ + useScopesServices: () => ({ + scopesSelectorService: { + closeAndApply: jest.fn(), + }, + }), +})); + +describe('ScopesTreeItemList', () => { + const mockFilterNode = jest.fn(); + const mockSelectScope = jest.fn(); + const mockDeselectScope = jest.fn(); + const mockToggleExpandedNode = jest.fn(); + + const defaultProps = { + anyChildExpanded: false, + lastExpandedNode: false, + loadingNodeName: undefined, + maxHeight: '100%', + selectedScopes: [] as SelectedScope[], + filterNode: mockFilterNode, + selectScope: mockSelectScope, + deselectScope: mockDeselectScope, + highlightedId: undefined, + id: 'test-tree', + toggleExpandedNode: mockToggleExpandedNode, + }; + + const createMockScopeNode = (name: string, parentName = 'parent'): ScopeNode => ({ + metadata: { name }, + spec: { + title: `Title ${name}`, + nodeType: 'leaf', + linkType: 'scope', + linkId: `scope-${name}`, + parentName, + }, + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should render nothing when items array is empty', () => { + const { container } = render(); + + expect(container.firstChild).toBeNull(); + }); + + it('should render tree items when nodes are available', () => { + const items: TreeNode[] = [ + { scopeNodeId: 'node-1', expanded: false, query: '' }, + { scopeNodeId: 'node-2', expanded: false, query: '' }, + ]; + + const scopeNodes: NodesMap = { + 'node-1': createMockScopeNode('node-1'), + 'node-2': createMockScopeNode('node-2'), + parent: { + metadata: { name: 'parent' }, + spec: { title: 'Parent', nodeType: 'container', parentName: '' }, + }, + }; + + render(); + + expect(screen.getByText('Title node-1')).toBeInTheDocument(); + expect(screen.getByText('Title node-2')).toBeInTheDocument(); + }); + + it('should skip rendering items when node data is not available in scopeNodes', () => { + const items: TreeNode[] = [ + { scopeNodeId: 'node-1', expanded: false, query: '' }, + { scopeNodeId: 'missing-node', expanded: false, query: '' }, // This node doesn't exist in scopeNodes + { scopeNodeId: 'node-2', expanded: false, query: '' }, + ]; + + const scopeNodes: NodesMap = { + 'node-1': createMockScopeNode('node-1'), + 'node-2': createMockScopeNode('node-2'), + parent: { + metadata: { name: 'parent' }, + spec: { title: 'Parent', nodeType: 'container', parentName: '' }, + }, + // 'missing-node' is intentionally not included + }; + + render(); + + // Should render the available nodes + expect(screen.getByText('Title node-1')).toBeInTheDocument(); + expect(screen.getByText('Title node-2')).toBeInTheDocument(); + + // Should NOT crash and should skip the missing node + expect(screen.queryByText('Title missing-node')).not.toBeInTheDocument(); + }); + + it('should handle all items having missing node data gracefully', () => { + const items: TreeNode[] = [ + { scopeNodeId: 'missing-1', expanded: false, query: '' }, + { scopeNodeId: 'missing-2', expanded: false, query: '' }, + ]; + + const scopeNodes: NodesMap = {}; + + // Should not crash + const { container } = render(); + + // Container should have the tree div but no visible items rendered inside + expect(container.querySelector('[role="tree"]')).toBeInTheDocument(); + expect(screen.queryByRole('treeitem')).not.toBeInTheDocument(); + }); + + it('should handle empty string scopeNodeId gracefully', () => { + const items: TreeNode[] = [ + { scopeNodeId: '', expanded: false, query: '' }, // Empty string scopeNodeId + ]; + + const scopeNodes: NodesMap = {}; + + // Should not crash + const { container } = render(); + + // Should have tree container but no items + expect(container.querySelector('[role="tree"]')).toBeInTheDocument(); + expect(screen.queryByRole('treeitem')).not.toBeInTheDocument(); + }); +}); diff --git a/public/app/features/scopes/selector/ScopesTreeItemList.tsx b/public/app/features/scopes/selector/ScopesTreeItemList.tsx index 33d3a4e2b53..e28a8cda214 100644 --- a/public/app/features/scopes/selector/ScopesTreeItemList.tsx +++ b/public/app/features/scopes/selector/ScopesTreeItemList.tsx @@ -47,15 +47,20 @@ export function ScopesTreeItemList({ const children = (
{items.map((childNode) => { + const node = scopeNodes[childNode.scopeNodeId]; + // Skip rendering if node data isn't available + if (!node) { + return null; + } const selected = - isNodeSelectable(scopeNodes[childNode.scopeNodeId]) && + isNodeSelectable(node) && selectedScopes.some((s) => { if (s.scopeNodeId) { // If we have scopeNodeId we only match based on that so even if the actual scope is the same we don't // mark different scopeNode as selected. return s.scopeNodeId === childNode.scopeNodeId; } else { - return s.scopeId === scopeNodes[childNode.scopeNodeId]?.spec.linkId; + return s.scopeId === node.spec.linkId; } }); return (