Scopes: Fix selector crashing on unavailable node (#115417)

* Fix crash * Add regression tests * Remove component mocking
2025-12-16 18:52:08 +02:00
parent 482bb6a2fb
commit 6dd711b6f2
5 changed files with 407 additions and 17 deletions
@@ -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(<ScopesTree {...defaultProps} selectedScopes={[]} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} selectedScopes={selectedScopes} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} selectedScopes={selectedScopes} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} selectedScopes={selectedScopes} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} scopeNodes={scopeNodes} selectedScopes={selectedScopes} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} selectedScopes={selectedScopes} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} scopeNodes={{}} />);
+
+      // 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(<ScopesTree {...defaultProps} tree={tree} scopeNodes={scopeNodes} />);
+
+      // 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();
+    });
+  });
+});
@@ -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({
@@ -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;
  }
@@ -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(<ScopesTreeItemList {...defaultProps} items={[]} scopeNodes={{}} />);
+
+    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(<ScopesTreeItemList {...defaultProps} items={items} scopeNodes={scopeNodes} />);
+
+    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(<ScopesTreeItemList {...defaultProps} items={items} scopeNodes={scopeNodes} />);
+
+    // 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(<ScopesTreeItemList {...defaultProps} items={items} scopeNodes={scopeNodes} />);
+
+    // 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(<ScopesTreeItemList {...defaultProps} items={items} scopeNodes={scopeNodes} />);
+
+    // Should have tree container but no items
+    expect(container.querySelector('[role="tree"]')).toBeInTheDocument();
+    expect(screen.queryByRole('treeitem')).not.toBeInTheDocument();
+  });
+});
@@ -47,15 +47,20 @@ export function ScopesTreeItemList({
  const children = (
    <div role="tree" id={id} className={anyChildExpanded ? styles.expandedContainer : undefined}>
      {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 (