From 84a07be6e4e1acd8f064c3b390c30188d5703afc Mon Sep 17 00:00:00 2001 From: Eric Shields Date: Wed, 26 Nov 2025 15:47:32 -0800 Subject: [PATCH] Chore: Finalize removal of updateNode & expandOrFilter (#114202) - Remove references to, and related private functions for, `updateNode` and `expandOrFilter` - Remove obsolete tests - Update all usages of `updateNode` to `filterNode` - Integrate `expandOrFilter` functionality into `filterNode` - Add profiler to `filterNode` - Add `.claude` to `.gitignore` IDE junk section - Unit tests for `toggleExpandedNode` and `filterNode` - Add profiler to `toggleExpandedNode` Fixes: https://github.com/grafana/grafana-operator-experience-squad/issues/1566 --- .gitignore | 1 + .../actions/scopeActions.test.tsx | 28 +- .../commandPalette/actions/scopeActions.tsx | 10 +- .../commandPalette/actions/scopesUtils.ts | 5 +- .../selector/ScopesSelectorService.test.ts | 536 ++++++++++++------ .../scopes/selector/ScopesSelectorService.ts | 134 ++--- 6 files changed, 425 insertions(+), 289 deletions(-) diff --git a/.gitignore b/.gitignore index f1a90d05693..5302a698c2f 100644 --- a/.gitignore +++ b/.gitignore @@ -71,6 +71,7 @@ public/css/*.min.css .vs/ .cursor/ .devcontainer/ +.claude/ .eslintcache .stylelintcache diff --git a/public/app/features/commandPalette/actions/scopeActions.test.tsx b/public/app/features/commandPalette/actions/scopeActions.test.tsx index 0d828d66a81..126877224de 100644 --- a/public/app/features/commandPalette/actions/scopeActions.test.tsx +++ b/public/app/features/commandPalette/actions/scopeActions.test.tsx @@ -22,7 +22,7 @@ jest.mock('./scopesUtils', () => { }); const mockScopeServicesState = { - updateNode: jest.fn(), + filterNode: jest.fn(), selectScope: jest.fn(), resetSelection: jest.fn(), nodes: {}, @@ -99,12 +99,12 @@ describe('useRegisterScopesActions', () => { }); it('should register scope tree actions and return scopesRow when scopes are selected', () => { - const mockUpdateNode = jest.fn(); + const mockFilterNode = jest.fn(); // First run with empty scopes in the scopes service (useScopeServicesState as jest.Mock).mockReturnValue({ ...mockScopeServicesState, - updateNode: mockUpdateNode, + filterNode: mockFilterNode, selectedScopes: [{ scopeId: 'scope1', name: 'Scope 1' }], }); @@ -112,14 +112,14 @@ describe('useRegisterScopesActions', () => { return useRegisterScopesActions('', jest.fn()); }); - expect(mockUpdateNode).toHaveBeenCalledWith('', true, ''); + expect(mockFilterNode).toHaveBeenCalledWith('', ''); expect(useRegisterActions).toHaveBeenLastCalledWith([rootScopeAction], [[rootScopeAction]]); expect(result.current.scopesRow).toBeDefined(); // Simulate loading of scopes in the service (useScopeServicesState as jest.Mock).mockReturnValue({ ...mockScopeServicesState, - updateNode: mockUpdateNode, + filterNode: mockFilterNode, selectedScopes: [{ scopeId: 'scope1', name: 'Scope 1' }], nodes, tree, @@ -151,12 +151,12 @@ describe('useRegisterScopesActions', () => { }); it('should load next level of scopes', () => { - const mockUpdateNode = jest.fn(); + const mockFilterNode = jest.fn(); // First run with empty scopes in the scopes service (useScopeServicesState as jest.Mock).mockReturnValue({ ...mockScopeServicesState, - updateNode: mockUpdateNode, + filterNode: mockFilterNode, nodes, tree, }); @@ -165,7 +165,7 @@ describe('useRegisterScopesActions', () => { return useRegisterScopesActions('', jest.fn(), 'scopes/scope1'); }); - expect(mockUpdateNode).toHaveBeenCalledWith('scope1', true, ''); + expect(mockFilterNode).toHaveBeenCalledWith('scope1', ''); }); it('does not return component if no scopes are selected', () => { @@ -259,12 +259,12 @@ describe('useRegisterScopesActions', () => { }); it('should not use global scope search when searching in some deeper scope category', async () => { - const mockUpdateNode = jest.fn(); + const mockFilterNode = jest.fn(); // First run with empty scopes in the scopes service (useScopeServicesState as jest.Mock).mockReturnValue({ ...mockScopeServicesState, - updateNode: mockUpdateNode, + filterNode: mockFilterNode, nodes, tree, }); @@ -273,17 +273,17 @@ describe('useRegisterScopesActions', () => { return useRegisterScopesActions('something', jest.fn(), 'scopes/scope1'); }); - expect(mockUpdateNode).toHaveBeenCalledWith('scope1', true, 'something'); + expect(mockFilterNode).toHaveBeenCalledWith('scope1', 'something'); expect(mockScopeServicesState.searchAllNodes).not.toHaveBeenCalled(); }); it('should not use global scope search if feature flag is off', async () => { config.featureToggles.scopeSearchAllLevels = false; - const mockUpdateNode = jest.fn(); + const mockFilterNode = jest.fn(); // First run with empty scopes in the scopes service (useScopeServicesState as jest.Mock).mockReturnValue({ ...mockScopeServicesState, - updateNode: mockUpdateNode, + filterNode: mockFilterNode, nodes, tree, }); @@ -292,7 +292,7 @@ describe('useRegisterScopesActions', () => { return useRegisterScopesActions('something', jest.fn(), ''); }); - expect(mockUpdateNode).toHaveBeenCalledWith('', true, 'something'); + expect(mockFilterNode).toHaveBeenCalledWith('', 'something'); expect(mockScopeServicesState.searchAllNodes).not.toHaveBeenCalled(); }); diff --git a/public/app/features/commandPalette/actions/scopeActions.tsx b/public/app/features/commandPalette/actions/scopeActions.tsx index bde41152237..e85d136d2f6 100644 --- a/public/app/features/commandPalette/actions/scopeActions.tsx +++ b/public/app/features/commandPalette/actions/scopeActions.tsx @@ -58,22 +58,22 @@ export function useRegisterScopesActions( * @param parentId */ function useScopeTreeActions(searchQuery: string, parentId?: string | null) { - const { updateNode, selectScope, resetSelection, nodes, tree, selectedScopes } = useScopeServicesState(); + const { filterNode, selectScope, resetSelection, nodes, tree, selectedScopes } = useScopeServicesState(); // Initialize the scopes the first time this runs and reset the scopes that were selected on unmount. useEffect(() => { - updateNode('', true, ''); + filterNode('', ''); resetSelection(); return () => { resetSelection(); }; - }, [updateNode, resetSelection]); + }, [filterNode, resetSelection]); // Load the next level of scopes when the parentId changes. useEffect(() => { const parentScopeId = !parentId || parentId === 'scopes' ? '' : last(parentId.split('/'))!; - updateNode(parentScopeId, true, searchQuery); - }, [updateNode, searchQuery, parentId]); + filterNode(parentScopeId, searchQuery); + }, [filterNode, searchQuery, parentId]); return useMemo( () => mapScopesNodesTreeToActions(nodes, tree!, selectedScopes, selectScope), diff --git a/public/app/features/commandPalette/actions/scopesUtils.ts b/public/app/features/commandPalette/actions/scopesUtils.ts index 2848569247a..d269a58abb1 100644 --- a/public/app/features/commandPalette/actions/scopesUtils.ts +++ b/public/app/features/commandPalette/actions/scopesUtils.ts @@ -14,7 +14,7 @@ export function useScopeServicesState() { const services = useScopesServices(); if (!services) { return { - updateNode: () => {}, + filterNode: () => Promise.resolve(), selectScope: () => {}, resetSelection: () => {}, searchAllNodes: () => Promise.resolve([]), @@ -32,7 +32,7 @@ export function useScopeServicesState() { }, }; } - const { updateNode, filterNode, selectScope, resetSelection, searchAllNodes, deselectScope, apply, getScopeNodes } = + const { filterNode, selectScope, resetSelection, searchAllNodes, deselectScope, apply, getScopeNodes } = services.scopesSelectorService; const selectorServiceState: ScopesSelectorServiceState | undefined = useObservable( services.scopesSelectorService.stateObservable ?? new Observable(), @@ -42,7 +42,6 @@ export function useScopeServicesState() { return { getScopeNodes, filterNode, - updateNode, selectScope, resetSelection, searchAllNodes, diff --git a/public/app/features/scopes/selector/ScopesSelectorService.test.ts b/public/app/features/scopes/selector/ScopesSelectorService.test.ts index 57ff735f019..2c89ea58fe5 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.test.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.test.ts @@ -107,162 +107,9 @@ describe('ScopesSelectorService', () => { service = new ScopesSelectorService(apiClient, dashboardsService, store); }); - describe('updateNode', () => { - it('should update node and fetch children when expanded', async () => { - await service.updateNode('', true, ''); - expect(service.state.nodes['test-scope-node']).toEqual(mockNode); - expect(service.state.tree).toMatchObject({ - children: { 'test-scope-node': { expanded: false, scopeNodeId: 'test-scope-node' } }, - expanded: true, - query: '', - scopeNodeId: '', - }); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: '' }); - }); - - it.skip('should update node query and fetch children when query changes', async () => { - await service.updateNode('', true, ''); // Expand first - // Simulate a change in the query - await service.updateNode('', true, 'new-qu'); - await service.updateNode('', true, 'new-query'); - expect(service.state.tree).toMatchObject({ - children: {}, - expanded: true, - query: 'new-query', - scopeNodeId: '', - }); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: 'new-query' }); - }); - - it('should not fetch children when node is collapsed and query is unchanged', async () => { - // First expand the node - await service.updateNode('', true, ''); - // Then collapse it - await service.updateNode('', false, ''); - // Only the first expansion should trigger fetchNodes - expect(apiClient.fetchNodes).toHaveBeenCalledTimes(1); - }); - - it.skip('should clear query on first expansion but keep it when filtering within populated node', async () => { - const mockChildNode: ScopeNode = { - metadata: { name: 'child-node' }, - spec: { linkId: 'child-scope', linkType: 'scope', parentName: '', nodeType: 'leaf', title: 'child-node' }, - }; - - apiClient.fetchNodes.mockResolvedValue([mockChildNode]); - - // Scenario 1: First expansion (no children yet) - clear query for unfiltered view - await service.updateNode('', true, 'search-query'); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: undefined }); - - // Parent query should be cleared and child nodes should have no query (first expansion) - expect(service.state.tree?.query).toBe(''); - let childTreeNode = service.state.tree?.children?.['child-node']; - expect(childTreeNode?.query).toBe(''); - - // Scenario 2: Filtering within node that already has children - await service.updateNode('', true, 'new-search'); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: 'new-search' }); - - // Parent and child nodes should have the filter query (filtering within existing children) - expect(service.state.tree?.query).toBe('new-search'); - childTreeNode = service.state.tree?.children?.['child-node']; - expect(childTreeNode?.query).toBe('new-search'); - - expect(apiClient.fetchNodes).toHaveBeenCalledTimes(2); - }); - - it.skip('should always reset query on any expansion', async () => { - const mockChildNode: ScopeNode = { - metadata: { name: 'child-node' }, - spec: { linkId: 'child-scope', linkType: 'scope', parentName: '', nodeType: 'leaf', title: 'child-node' }, - }; - - apiClient.fetchNodes.mockResolvedValue([mockChildNode]); - - // First expansion with any query should reset parent query and not pass query to API - await service.updateNode('', true, 'some-search-query'); - - // Verify query is reset and API called without query for first expansion - expect(service.state.tree?.query).toBe(''); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: undefined }); - expect(service.state.tree?.children?.['child-node']?.query).toBe(''); - }); - - it.skip('should handle query reset correctly for nested levels beyond root', async () => { - // Set up mock nodes for multi-level hierarchy - const mockParentNode: ScopeNode = { - metadata: { name: 'parent-container' }, - spec: { linkId: '', linkType: 'scope', parentName: '', nodeType: 'container', title: 'Parent Container' }, - }; - - const mockChildNode: ScopeNode = { - metadata: { name: 'child-container' }, - spec: { - linkId: '', - linkType: 'scope', - parentName: 'parent-container', - nodeType: 'container', - title: 'Child Container', - }, - }; - - const mockGrandchildNode: ScopeNode = { - metadata: { name: 'grandchild-leaf' }, - spec: { - linkId: 'leaf-scope', - linkType: 'scope', - parentName: 'child-container', - nodeType: 'leaf', - title: 'Grandchild Leaf', - }, - }; - - // Mock different responses for different parent nodes - apiClient.fetchNodes.mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { - if (options.parent === '') { - return Promise.resolve([mockParentNode]); - } else if (options.parent === 'parent-container') { - return Promise.resolve([mockChildNode]); - } else if (options.parent === 'child-container') { - return Promise.resolve([mockGrandchildNode]); - } - return Promise.resolve([]); - }); - - // Step 1: Expand root node with search query - await service.updateNode('', true, 'search-query'); - - // Root should have query reset, API called without query - expect(service.state.tree?.query).toBe(''); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: '', query: undefined }); - expect(service.state.tree?.children?.['parent-container']?.query).toBe(''); - - // Step 2: Expand first-level child with search query - await service.updateNode('parent-container', true, 'open-search-query'); - - // First-level child should have query reset, API called without query - const parentContainer = service.state.tree?.children?.['parent-container']; - expect(parentContainer?.query).toBe(''); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: 'parent-container', query: undefined }); - expect(parentContainer?.children?.['child-container']?.query).toBe(''); - - // Step 3: Now filter within the first-level child (second call to same node) - await service.updateNode('parent-container', true, 'filter-search'); - - // Now both parent and children should show the filter query since we're filtering within existing children - const newParentContainer = service.state.tree?.children?.['parent-container']; - expect(newParentContainer?.query).toBe('filter-search'); - expect(apiClient.fetchNodes).toHaveBeenCalledWith({ parent: 'parent-container', query: 'filter-search' }); - expect(newParentContainer?.children?.['child-container']?.query).toBe('filter-search'); - - expect(apiClient.fetchNodes).toHaveBeenCalledTimes(3); - }); - }); - describe('selectScope and deselectScope', () => { beforeEach(async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); }); it('should select a scope', async () => { @@ -311,7 +158,7 @@ describe('ScopesSelectorService', () => { it('should set parent node for recent scopes', async () => { // Load mock node - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.changeScopes(['test-scope'], 'test-scope-node'); expect(service.state.appliedScopes).toEqual([{ scopeId: 'test-scope', parentNodeId: 'test-scope-node' }]); @@ -363,7 +210,7 @@ describe('ScopesSelectorService', () => { describe('closeAndApply', () => { it('should close the selector and apply the selected scopes', async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.selectScope('test-scope-node'); await service.closeAndApply(); expect(service.state.opened).toBe(false); @@ -373,6 +220,7 @@ describe('ScopesSelectorService', () => { describe('apply', () => { it('should apply the selected scopes without closing the selector', async () => { + await service.filterNode('', ''); await service.open(); await service.selectScope('test-scope-node'); await service.apply(); @@ -391,7 +239,7 @@ describe('ScopesSelectorService', () => { describe('removeAllScopes', () => { it('should remove all selected and applied scopes', async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.selectScope('test-scope-node'); await service.apply(); await service.removeAllScopes(); @@ -399,7 +247,7 @@ describe('ScopesSelectorService', () => { }); it('should clear navigation scope when removing all scopes', async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.selectScope('test-scope-node'); await service.apply(); await service.removeAllScopes(); @@ -429,7 +277,7 @@ describe('ScopesSelectorService', () => { describe('getRecentScopes', () => { it('should parse and filter scopes', async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.selectScope('test-scope-node'); await service.apply(); storeValue[RECENT_SCOPES_KEY] = JSON.stringify([[mockScope2], [mockScope]]); @@ -439,7 +287,7 @@ describe('ScopesSelectorService', () => { }); it('should work with old version', async () => { - await service.updateNode('', true, ''); + await service.filterNode('', ''); await service.selectScope('test-scope-node'); await service.apply(); storeValue[RECENT_SCOPES_KEY] = JSON.stringify([ @@ -615,9 +463,347 @@ describe('ScopesSelectorService', () => { }); }); + describe('toggleExpandedNode', () => { + const expandableNode: ScopeNode = { + metadata: { name: 'expandable-node' }, + spec: { + linkId: '', + linkType: undefined, + parentName: '', + nodeType: 'container', + title: 'Expandable Node', + }, + }; + + const childNode: ScopeNode = { + metadata: { name: 'child-node' }, + spec: { + linkId: 'child-scope', + linkType: 'scope', + parentName: 'expandable-node', + nodeType: 'leaf', + title: 'Child Node', + }, + }; + + const leafNode: ScopeNode = { + metadata: { name: 'leaf-node' }, + spec: { + linkId: 'leaf-scope', + linkType: 'scope', + parentName: '', + nodeType: 'leaf', + title: 'Leaf Node', + }, + }; + + beforeEach(async () => { + // Mock fetchNodes to return different nodes based on parent + apiClient.fetchNodes = jest + .fn() + .mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '') { + return [expandableNode, leafNode]; + } else if (options.parent === 'expandable-node') { + return [childNode]; + } + return []; + }); + + // Load root nodes + await service.filterNode('', ''); + }); + + it('should expand a collapsed node and load its children', async () => { + // Node should start collapsed + expect(service.state.tree?.children?.['expandable-node']?.expanded).toBe(false); + + // Expand the node + await service.toggleExpandedNode('expandable-node'); + + // Node should now be expanded + expect(service.state.tree?.children?.['expandable-node']?.expanded).toBe(true); + // Children should be loaded + expect(service.state.tree?.children?.['expandable-node']?.children).toBeDefined(); + expect(service.state.tree?.children?.['expandable-node']?.children?.['child-node']).toBeDefined(); + }); + + it('should collapse an expanded node', async () => { + // First expand the node + await service.toggleExpandedNode('expandable-node'); + expect(service.state.tree?.children?.['expandable-node']?.expanded).toBe(true); + + // Now collapse it + await service.toggleExpandedNode('expandable-node'); + expect(service.state.tree?.children?.['expandable-node']?.expanded).toBe(false); + }); + + it('should reset query to empty string when toggling', async () => { + // First filter with a query + await service.filterNode('expandable-node', 'test-query'); + expect(service.state.tree?.children?.['expandable-node']?.query).toBe('test-query'); + + // Toggle the node + await service.toggleExpandedNode('expandable-node'); + + // Query should be reset + expect(service.state.tree?.children?.['expandable-node']?.query).toBe(''); + }); + + it('should throw error when node not found in tree', async () => { + await expect(service.toggleExpandedNode('non-existent-node')).rejects.toThrow( + 'Node non-existent-node not found in tree' + ); + }); + + it('should throw error when trying to toggle a non-expandable node', async () => { + await expect(service.toggleExpandedNode('leaf-node')).rejects.toThrow( + 'Trying to expand node at id leaf-node that is not expandable' + ); + }); + + it('should reload parent children when collapsing', async () => { + const fetchNodesSpy = jest.spyOn(apiClient, 'fetchNodes'); + + // Expand then collapse + await service.toggleExpandedNode('expandable-node'); + fetchNodesSpy.mockClear(); + + await service.toggleExpandedNode('expandable-node'); + + // Should reload parent's (root) children + expect(fetchNodesSpy).toHaveBeenCalledWith({ parent: '', query: '' }); + }); + + it('should reload parent children with parent query when collapsing', async () => { + // First filter the root with a query + await service.filterNode('', 'parent-query'); + + // Expand a node + await service.toggleExpandedNode('expandable-node'); + + const fetchNodesSpy = jest.spyOn(apiClient, 'fetchNodes'); + + // Collapse the node + await service.toggleExpandedNode('expandable-node'); + + // Should reload parent's children with parent's query + expect(fetchNodesSpy).toHaveBeenCalledWith({ parent: '', query: 'parent-query' }); + }); + }); + + describe('filterNode', () => { + const containerNode: ScopeNode = { + metadata: { name: 'container-node' }, + spec: { + linkId: '', + linkType: undefined, + parentName: '', + nodeType: 'container', + title: 'Container Node', + }, + }; + + const filteredChild: ScopeNode = { + metadata: { name: 'filtered-child' }, + spec: { + linkId: 'filtered-scope', + linkType: 'scope', + parentName: 'container-node', + nodeType: 'leaf', + title: 'Filtered Child', + }, + }; + + const leafNode: ScopeNode = { + metadata: { name: 'leaf-node-2' }, + spec: { + linkId: 'leaf-scope-2', + linkType: 'scope', + parentName: '', + nodeType: 'leaf', + title: 'Leaf Node 2', + }, + }; + + beforeEach(async () => { + // Mock fetchNodes to return different nodes based on query + apiClient.fetchNodes = jest + .fn() + .mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '' && !options.query) { + return [containerNode, leafNode]; + } else if (options.parent === 'container-node' && options.query === 'test-filter') { + return [filteredChild]; + } else if (options.parent === 'container-node' && !options.query) { + return [filteredChild]; + } + return []; + }); + + // Load root nodes + await service.filterNode('', ''); + }); + + it('should filter node with non-empty query', async () => { + await service.filterNode('container-node', 'test-filter'); + + // Node should be expanded + expect(service.state.tree?.children?.['container-node']?.expanded).toBe(true); + // Query should be set + expect(service.state.tree?.children?.['container-node']?.query).toBe('test-filter'); + }); + + it('should load children with the query parameter', async () => { + const fetchNodesSpy = jest.spyOn(apiClient, 'fetchNodes'); + + await service.filterNode('container-node', 'my-query'); + + expect(fetchNodesSpy).toHaveBeenCalledWith({ parent: 'container-node', query: 'my-query' }); + }); + + it('should set expanded to true when filtering', async () => { + // Node starts collapsed + expect(service.state.tree?.children?.['container-node']?.expanded).toBe(false); + + await service.filterNode('container-node', 'test-filter'); + + // Should be expanded after filtering + expect(service.state.tree?.children?.['container-node']?.expanded).toBe(true); + }); + + it('should throw error when node not found', async () => { + await expect(service.filterNode('non-existent-node', 'query')).rejects.toThrow( + 'Trying to filter node at path or id non-existent-node not found' + ); + }); + + it('should throw error when trying to filter a non-expandable node', async () => { + await expect(service.filterNode('leaf-node-2', 'query')).rejects.toThrow( + 'Trying to filter node at id leaf-node-2 that is not expandable' + ); + }); + + it('should handle multiple calls with different queries', async () => { + // First filter + await service.filterNode('container-node', 'first-query'); + expect(service.state.tree?.children?.['container-node']?.query).toBe('first-query'); + + // Second filter with different query + await service.filterNode('container-node', 'second-query'); + expect(service.state.tree?.children?.['container-node']?.query).toBe('second-query'); + + // Third filter with empty query + await service.filterNode('container-node', ''); + expect(service.state.tree?.children?.['container-node']?.query).toBe(''); + }); + + it('should start profiler interaction', async () => { + const profiler = { + startInteraction: jest.fn(), + stopInteraction: jest.fn(), + }; + + // Create new service with profiler + const serviceWithProfiler = new ScopesSelectorService(apiClient, dashboardsService, store, profiler as never); + + await serviceWithProfiler.filterNode('', ''); + + expect(profiler.startInteraction).toHaveBeenCalledWith('scopeNodeFilter'); + expect(profiler.stopInteraction).toHaveBeenCalled(); + }); + + it('should stop profiler even when error is thrown', async () => { + const profiler = { + startInteraction: jest.fn(), + stopInteraction: jest.fn(), + }; + + const serviceWithProfiler = new ScopesSelectorService(apiClient, dashboardsService, store, profiler as never); + + // Load initial nodes + await serviceWithProfiler.filterNode('', ''); + + // Try to filter a non-existent node + await expect(serviceWithProfiler.filterNode('non-existent', 'query')).rejects.toThrow(); + + // Profiler should still be stopped + expect(profiler.stopInteraction).toHaveBeenCalled(); + }); + }); + + describe('interaction between toggleExpandedNode and filterNode', () => { + const expandableNode: ScopeNode = { + metadata: { name: 'interaction-node' }, + spec: { + linkId: '', + linkType: undefined, + parentName: '', + nodeType: 'container', + title: 'Interaction Node', + }, + }; + + const childNode: ScopeNode = { + metadata: { name: 'interaction-child' }, + spec: { + linkId: 'child-scope', + linkType: 'scope', + parentName: 'interaction-node', + nodeType: 'leaf', + title: 'Child Node', + }, + }; + + beforeEach(async () => { + apiClient.fetchNodes = jest + .fn() + .mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '') { + return [expandableNode]; + } else if (options.parent === 'interaction-node') { + return [childNode]; + } + return []; + }); + + await service.filterNode('', ''); + }); + + it('should clear query when toggleExpandedNode is called after filterNode', async () => { + // Filter with a query + await service.filterNode('interaction-node', 'test-query'); + expect(service.state.tree?.children?.['interaction-node']?.query).toBe('test-query'); + + // Toggle should clear the query + await service.toggleExpandedNode('interaction-node'); + expect(service.state.tree?.children?.['interaction-node']?.query).toBe(''); + }); + + it('should set query when filterNode is called after toggleExpandedNode', async () => { + // First toggle (expand) + await service.toggleExpandedNode('interaction-node'); + expect(service.state.tree?.children?.['interaction-node']?.query).toBe(''); + + // Filter should set the query + await service.filterNode('interaction-node', 'new-query'); + expect(service.state.tree?.children?.['interaction-node']?.query).toBe('new-query'); + }); + + it('should maintain expanded state when filtering an already expanded node', async () => { + // Expand the node + await service.toggleExpandedNode('interaction-node'); + expect(service.state.tree?.children?.['interaction-node']?.expanded).toBe(true); + + // Filter should keep it expanded + await service.filterNode('interaction-node', 'query'); + expect(service.state.tree?.children?.['interaction-node']?.expanded).toBe(true); + }); + }); + describe('redirect on scope selection', () => { it('should redirect to the first scopeNavigation with /d/ URL when current URL is not a scopeNavigation', async () => { - const mockNavigations: ScopeNavigation[] = [ + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', @@ -632,8 +818,6 @@ describe('ScopesSelectorService', () => { }, }, ]; - - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); await service.changeScopes(['test-scope']); @@ -642,7 +826,7 @@ describe('ScopesSelectorService', () => { }); it('should NOT redirect when the first scopeNavigation does not contain /d/ (e.g., logs drilldown)', async () => { - const mockNavigations: ScopeNavigation[] = [ + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', @@ -657,8 +841,6 @@ describe('ScopesSelectorService', () => { }, }, ]; - - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); await service.changeScopes(['test-scope']); @@ -667,7 +849,7 @@ describe('ScopesSelectorService', () => { }); it('should NOT redirect when current URL matches a scopeNavigation', async () => { - const mockNavigations: ScopeNavigation[] = [ + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', @@ -682,8 +864,6 @@ describe('ScopesSelectorService', () => { }, }, ]; - - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/d/dashboard1' }); await service.changeScopes(['test-scope']); @@ -701,7 +881,7 @@ describe('ScopesSelectorService', () => { }); it('should NOT redirect when scopeNavigation does not have a url property', async () => { - const mockNavigations = [ + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', @@ -716,8 +896,6 @@ describe('ScopesSelectorService', () => { }, }, ] as unknown as ScopeNavigation[]; - - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); await service.changeScopes(['test-scope']); @@ -726,7 +904,7 @@ describe('ScopesSelectorService', () => { }); it('should handle multiple scopeNavigations and redirect to the first dashboard one', async () => { - const mockNavigations: ScopeNavigation[] = [ + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', @@ -754,8 +932,6 @@ describe('ScopesSelectorService', () => { }, }, ]; - - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); await service.changeScopes(['test-scope']); @@ -790,7 +966,7 @@ describe('ScopesSelectorService', () => { }); // First update the node to populate the service state - await service.updateNode('', true, ''); + await service.filterNode('', ''); // Then select the scope to set scopeNodeId in selectedScopes await service.selectScope('test-scope-node'); @@ -837,7 +1013,7 @@ describe('ScopesSelectorService', () => { (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); // First update the node to populate the service state - await service.updateNode('', true, ''); + await service.filterNode('', ''); // Then select the scope to set scopeNodeId in selectedScopes await service.selectScope('test-scope-node'); @@ -851,16 +1027,14 @@ describe('ScopesSelectorService', () => { }); it('should fall back to scope navigation when scope node is undefined', async () => { - const mockNavigations: ScopeNavigation[] = [ + // Don't add the node to the service state, so it will be undefined + dashboardsService.state.scopeNavigations = [ { spec: { scope: 'test-scope', url: '/d/dashboard1' }, status: { title: 'Dashboard 1', groups: [] }, metadata: { name: 'dashboard1' }, }, ]; - - // Don't add the node to the service state, so it will be undefined - dashboardsService.state.scopeNavigations = mockNavigations; (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); await service.changeScopes(['test-scope']); diff --git a/public/app/features/scopes/selector/ScopesSelectorService.ts b/public/app/features/scopes/selector/ScopesSelectorService.ts index 14a16be69ac..9dea729ff88 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.ts @@ -129,81 +129,38 @@ export class ScopesSelectorService extends ScopesServiceBase { - const path = getPathOfNode(scopeNodeId, this.state.nodes); - const nodeToToggle = treeNodeAtPath(this.state.tree!, path); - - if (!nodeToToggle) { - throw new Error(`Node ${scopeNodeId} not found in tree`); - } - - if (nodeToToggle.scopeNodeId !== '' && !isNodeExpandable(this.state.nodes[nodeToToggle.scopeNodeId])) { - throw new Error(`Trying to expand node at id ${scopeNodeId} that is not expandable`); - } - - const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { - treeNode.expanded = !nodeToToggle.expanded; - treeNode.query = ''; - }); - - this.updateState({ tree: newTree }); - // If we are collapsing, we need to make sure that all the parent's children are avilable - if (nodeToToggle.expanded === true) { - const parentPath = path.slice(0, -1); - const parentNode = treeNodeAtPath(this.state.tree!, parentPath); - if (parentNode) { - await this.loadNodeChildren(parentPath, parentNode, parentNode.query); - } - } else { - await this.loadNodeChildren(path, nodeToToggle); - } - }; - - public filterNode = async (scopeNodeId: string, query: string) => { - const path = getPathOfNode(scopeNodeId, this.state.nodes); - const nodeToFilter = treeNodeAtPath(this.state.tree!, path); - - if (!nodeToFilter) { - throw new Error(`Trying to filter node at path or id ${scopeNodeId} not found`); - } - - if (nodeToFilter.scopeNodeId !== '' && !isNodeExpandable(this.state.nodes[nodeToFilter.scopeNodeId])) { - throw new Error(`Trying to filter node at id ${scopeNodeId} that is not expandable`); - } - - const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { - treeNode.expanded = true; - treeNode.query = query; - }); - this.updateState({ tree: newTree }); - - await this.loadNodeChildren(path, nodeToFilter, query); - }; - - private expandOrFilterNode = async (scopeNodeId: string, query?: string) => { - this.interactionProfiler?.startInteraction('scopeNodeDiscovery'); - - const path = getPathOfNode(scopeNodeId, this.state.nodes); - - const nodeToExpand = treeNodeAtPath(this.state.tree!, path); + this.interactionProfiler?.startInteraction('scopeToggleExpandedNode'); try { - if (!nodeToExpand) { + const path = getPathOfNode(scopeNodeId, this.state.nodes); + const nodeToToggle = treeNodeAtPath(this.state.tree!, path); + + if (!nodeToToggle) { throw new Error(`Node ${scopeNodeId} not found in tree`); } - if (nodeToExpand.scopeNodeId !== '' && !isNodeExpandable(this.state.nodes[nodeToExpand.scopeNodeId])) { + if (nodeToToggle.scopeNodeId !== '' && !isNodeExpandable(this.state.nodes[nodeToToggle.scopeNodeId])) { throw new Error(`Trying to expand node at id ${scopeNodeId} that is not expandable`); } - if (!nodeToExpand.expanded || nodeToExpand.query !== query) { - const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { - treeNode.expanded = true; - treeNode.query = query || ''; - }); - this.updateState({ tree: newTree }); + const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { + treeNode.expanded = !nodeToToggle.expanded; + treeNode.query = ''; + }); - await this.loadNodeChildren(path, nodeToExpand, query); + this.updateState({ tree: newTree }); + // If we are collapsing, we need to make sure that all the parent's children are available + if (nodeToToggle.expanded) { + const parentPath = path.slice(0, -1); + const parentNode = treeNodeAtPath(this.state.tree!, parentPath); + if (parentNode) { + await this.loadNodeChildren(parentPath, parentNode, parentNode.query); + } + } else { + await this.loadNodeChildren(path, nodeToToggle); } + // Catch and throw error so we can ensure the profiler is stopped + // todo: leverage component-level } catch (error) { throw error; } finally { @@ -211,20 +168,34 @@ export class ScopesSelectorService extends ScopesServiceBase { - const path = getPathOfNode(scopeNodeId, this.state.nodes); + public filterNode = async (scopeNodeId: string, query: string) => { + this.interactionProfiler?.startInteraction('scopeNodeFilter'); - const nodeToCollapse = treeNodeAtPath(this.state.tree!, path); + try { + const path = getPathOfNode(scopeNodeId, this.state.nodes); + const nodeToFilter = treeNodeAtPath(this.state.tree!, path); - if (!nodeToCollapse) { - throw new Error(`Trying to collapse node at path or id ${scopeNodeId} not found`); + if (!nodeToFilter) { + throw new Error(`Trying to filter node at path or id ${scopeNodeId} not found`); + } + + if (nodeToFilter.scopeNodeId !== '' && !isNodeExpandable(this.state.nodes[nodeToFilter.scopeNodeId])) { + throw new Error(`Trying to filter node at id ${scopeNodeId} that is not expandable`); + } + + const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { + treeNode.expanded = true; + treeNode.query = query; + }); + this.updateState({ tree: newTree }); + + await this.loadNodeChildren(path, nodeToFilter, query); + // Catch and throw error so we can ensure the profiler is stopped + } catch (error) { + throw error; + } finally { + this.interactionProfiler?.stopInteraction(); } - - const newTree = modifyTreeNodeAtPath(this.state.tree!, path, (treeNode) => { - treeNode.expanded = false; - treeNode.query = ''; - }); - this.updateState({ tree: newTree }); }; private loadNodeChildren = async (path: string[], treeNode: TreeNode, query?: string) => { @@ -332,15 +303,6 @@ export class ScopesSelectorService extends ScopesServiceBase { - if (expanded) { - return this.expandOrFilterNode(scopeNodeId, query); - } - return this.collapseNode(scopeNodeId); - }; - changeScopes = (scopeNames: string[], parentNodeId?: string, scopeNodeId?: string, redirectOnApply?: boolean) => { return this.applyScopes( scopeNames.map((id, index) => ({ @@ -494,7 +456,7 @@ export class ScopesSelectorService extends ScopesServiceBase { if (!this.state.tree?.children || Object.keys(this.state.tree?.children).length === 0) { - await this.expandOrFilterNode(''); + await this.filterNode('', ''); } // First close all nodes