From 6746c978b4fb881b74c2e062d69751191ddda668 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Tue, 9 Dec 2025 13:44:00 +0100 Subject: [PATCH] Scopes: Resolve path directly from leaf node bugfix (#114507) * Resolve path directly from leaf node * Add childrenLoaded field * Add tests and remove parentNodeId from changeScopes * Move parentNodeId patameter order * Resotre call order * Undo superflous change * Add comments * Make sure childrenLoaded state is properly set default to false * Reference parent path * Look for parent in state and fetch scopeNode if it is not avilable * Check for undefined * Add mock to test * Set scopeNodeId with recent scopes * Improve test selector * Add scope node endpoint to mocks * Never set childrenLoaded to true when inserting * Remove unused import * Pass on the already set childrenLoaded value * Fix test --- .../app/features/scopes/ScopesService.test.ts | 21 +- public/app/features/scopes/ScopesService.ts | 12 +- .../features/scopes/selector/RecentScopes.tsx | 5 +- .../features/scopes/selector/ScopesInput.tsx | 6 +- .../scopes/selector/ScopesSelector.tsx | 4 +- .../selector/ScopesSelectorService.test.ts | 193 ++++++++++++++++++ .../scopes/selector/ScopesSelectorService.ts | 67 ++++-- .../features/scopes/selector/ScopesTree.tsx | 2 +- .../scopes/selector/scopesTreeUtils.test.ts | 115 +++++++++++ .../scopes/selector/scopesTreeUtils.ts | 3 + public/app/features/scopes/selector/types.ts | 6 +- .../scopes/selector/useScopesHighlighting.tsx | 2 +- .../features/scopes/tests/selector.test.ts | 3 +- public/app/features/scopes/tests/tree.test.ts | 41 +++- .../app/features/scopes/tests/utils/mocks.ts | 6 + 15 files changed, 443 insertions(+), 43 deletions(-) diff --git a/public/app/features/scopes/ScopesService.test.ts b/public/app/features/scopes/ScopesService.test.ts index 6b360f853e3..51c50cc9ff9 100644 --- a/public/app/features/scopes/ScopesService.test.ts +++ b/public/app/features/scopes/ScopesService.test.ts @@ -117,7 +117,8 @@ describe('ScopesService', () => { expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], undefined, 'node1', false); }); - it('should read scope_parent for backward compatibility', () => { + // TODO: remove when parentNodeId is removed + it('should ignore scope_parent from URL (only used for recent scopes)', () => { locationService.getLocation = jest.fn().mockReturnValue({ pathname: '/test', search: '?scopes=scope1&scope_parent=parent1', @@ -125,10 +126,12 @@ describe('ScopesService', () => { service = new ScopesService(selectorService, dashboardsService, locationService); - expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], 'parent1', undefined, false); + // parentNodeId should be undefined since we don't read it from URL + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], undefined, undefined, false); }); - it('should prefer scope_node when both scope_node and scope_parent exist', () => { + // TODO: remove when parentNodeId is removed + it('should only use scope_node when both scope_node and scope_parent exist in URL', () => { locationService.getLocation = jest.fn().mockReturnValue({ pathname: '/test', search: '?scopes=scope1&scope_node=node1&scope_parent=parent1', @@ -136,9 +139,9 @@ describe('ScopesService', () => { service = new ScopesService(selectorService, dashboardsService, locationService); - // Should call with parent1 as parentNodeId and node1 as scopeNodeId - expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], 'parent1', 'node1', false); - // Should preload node1 (not parent1) + // Should only use scopeNodeId from URL, parentNodeId is undefined + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], undefined, 'node1', false); + // Should preload node1 expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('node1', expect.anything()); }); @@ -153,7 +156,8 @@ describe('ScopesService', () => { expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('node1', expect.anything()); }); - it('should fallback to preload scope_parent when scope_node is not provided', () => { + // TODO: remove when parentNodeId is removed + it('should not preload when only scope_parent is in URL', () => { locationService.getLocation = jest.fn().mockReturnValue({ pathname: '/test', search: '?scopes=scope1&scope_parent=parent1', @@ -161,7 +165,8 @@ describe('ScopesService', () => { service = new ScopesService(selectorService, dashboardsService, locationService); - expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('parent1', expect.anything()); + // Should not preload since we don't read scope_parent from URL + expect(selectorService.resolvePathToRoot).not.toHaveBeenCalled(); }); it('should handle multiple scopes from URL', () => { diff --git a/public/app/features/scopes/ScopesService.ts b/public/app/features/scopes/ScopesService.ts index 36edcc94b13..1d0be39f56c 100644 --- a/public/app/features/scopes/ScopesService.ts +++ b/public/app/features/scopes/ScopesService.ts @@ -71,18 +71,16 @@ export class ScopesService implements ScopesContextValue { // Init from the URL when we first load const queryParams = new URLSearchParams(locationService.getLocation().search); const scopeNodeId = queryParams.get('scope_node'); - // TODO: figure out when to remove this. scope_parent is for backward compatibility only - const parentNodeId = queryParams.get('scope_parent'); const navigationScope = queryParams.get('navigation_scope'); if (navigationScope) { this.dashboardsService.setNavigationScope(navigationScope); } - this.changeScopes(queryParams.getAll('scopes'), parentNodeId ?? undefined, scopeNodeId ?? undefined); + this.changeScopes(queryParams.getAll('scopes'), undefined, scopeNodeId ?? undefined); - // Pre-load scope node (which loads parent too) or fallback to parent node for old URLs - const nodeToPreload = scopeNodeId ?? parentNodeId; + // Pre-load scope node (which loads parent too) + const nodeToPreload = scopeNodeId; if (nodeToPreload) { this.selectorService.resolvePathToRoot(nodeToPreload, this.selectorService.state.tree!).catch((error) => { console.error('Failed to pre-load node path', error); @@ -100,8 +98,6 @@ export class ScopesService implements ScopesContextValue { const scopes = queryParams.getAll('scopes'); const scopeNodeId = queryParams.get('scope_node'); - // scope_parent is for backward compatibility only - const parentNodeId = queryParams.get('scope_parent'); // Check if new scopes are different from the old scopes const currentScopes = this.selectorService.state.appliedScopes.map((scope) => scope.scopeId); @@ -109,7 +105,7 @@ export class ScopesService implements ScopesContextValue { // We only update scopes but never delete them. This is to keep the scopes in memory if user navigates to // page that does not use scopes (like from dashboard to dashboard list back to dashboard). If user // changes the URL directly, it would trigger a reload so scopes would still be reset. - this.changeScopes(scopes, parentNodeId ?? undefined, scopeNodeId ?? undefined); + this.changeScopes(scopes, undefined, scopeNodeId ?? undefined); } }) ); diff --git a/public/app/features/scopes/selector/RecentScopes.tsx b/public/app/features/scopes/selector/RecentScopes.tsx index 66771f635fb..a13ed97c275 100644 --- a/public/app/features/scopes/selector/RecentScopes.tsx +++ b/public/app/features/scopes/selector/RecentScopes.tsx @@ -9,7 +9,7 @@ import { RecentScope } from './types'; interface RecentScopesProps { recentScopes: RecentScope[][]; - onSelect: (scopeIds: string[], parentNodeId?: string) => void; + onSelect: (scopeIds: string[], parentNodeId?: string, scopeNodeId?: string) => void; } export const RecentScopes = ({ recentScopes, onSelect }: RecentScopesProps) => { @@ -45,7 +45,8 @@ export const RecentScopes = ({ recentScopes, onSelect }: RecentScopesProps) => { onClick={() => { onSelect( recentScopeSet.map((s) => s.metadata.name), - recentScopeSet[0]?.parentNode?.metadata?.name + recentScopeSet[0]?.parentNode?.metadata?.name, + recentScopeSet[0]?.scopeNodeId ); }} > diff --git a/public/app/features/scopes/selector/ScopesInput.tsx b/public/app/features/scopes/selector/ScopesInput.tsx index ce22820b32d..a180c523e6b 100644 --- a/public/app/features/scopes/selector/ScopesInput.tsx +++ b/public/app/features/scopes/selector/ScopesInput.tsx @@ -32,12 +32,12 @@ export function ScopesInput({ onRemoveAllClick, }: ScopesInputProps) { const scopeNodeId = appliedScopes[0]?.scopeNodeId; - const parentNodeIdFromUrl = appliedScopes[0]?.parentNodeId; const styles = useStyles2(getStyles); + const parentNodeIdFromRecentScopes = appliedScopes[0]?.parentNodeId; // This is only set from recent scopes TODO: remove after recent scopes refactor const { node: scopeNode, isLoading: scopeNodeLoading } = useScopeNode(scopeNodeId); - // Get parent from scope node if available, otherwise use parentNodeId from URL (for backward compatibility) - const parentNodeId = scopeNode?.spec.parentName ?? parentNodeIdFromUrl; + // Get parent from scope node if available, otherwise fallback to parent + const parentNodeId = scopeNode?.spec.parentName ?? parentNodeIdFromRecentScopes; const { node: parentNode, isLoading: parentNodeLoading } = useScopeNode(parentNodeId); // Prioritize scope node subtitle over parent node title diff --git a/public/app/features/scopes/selector/ScopesSelector.tsx b/public/app/features/scopes/selector/ScopesSelector.tsx index cdb28acddf8..bc89c748814 100644 --- a/public/app/features/scopes/selector/ScopesSelector.tsx +++ b/public/app/features/scopes/selector/ScopesSelector.tsx @@ -132,8 +132,8 @@ export const ScopesSelector = () => { selectScope={selectScope} deselectScope={deselectScope} toggleExpandedNode={toggleExpandedNode} - onRecentScopesSelect={(scopeIds: string[], parentNodeId?: string) => { - scopesSelectorService.changeScopes(scopeIds, parentNodeId); + onRecentScopesSelect={(scopeIds: string[], parentNodeId?: string, scopeNodeId?: string) => { + scopesSelectorService.changeScopes(scopeIds, parentNodeId, scopeNodeId); scopesSelectorService.closeAndReset(); }} /> diff --git a/public/app/features/scopes/selector/ScopesSelectorService.test.ts b/public/app/features/scopes/selector/ScopesSelectorService.test.ts index 2c89ea58fe5..673046adc0f 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.test.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.test.ts @@ -15,6 +15,14 @@ jest.mock('@grafana/runtime', () => ({ push: jest.fn(), getLocation: jest.fn(), }, + config: { + ...jest.requireActual('@grafana/runtime').config, + + featureToggles: { + ...jest.requireActual('@grafana/runtime').config.featureToggles, + useScopeSingleNodeEndpoint: true, + }, + }, })); describe('ScopesSelectorService', () => { @@ -69,6 +77,7 @@ describe('ScopesSelectorService', () => { }), fetchDashboards: jest.fn().mockResolvedValue([]), fetchScopeNavigations: jest.fn().mockResolvedValue([]), + fetchScopeNode: jest.fn().mockResolvedValue(mockNode), } as unknown as jest.Mocked; dashboardsService = { @@ -197,6 +206,190 @@ describe('ScopesSelectorService', () => { await service.open(); expect(service.state.opened).toBe(true); }); + + it('should use scopeNodeId to resolve path when opening selector', async () => { + const parentNode: ScopeNode = { + metadata: { name: 'parent-container' }, + spec: { + linkId: '', + linkType: 'scope', + //parentName: '', + nodeType: 'container', + title: 'Parent Container', + }, + }; + + const childNode: ScopeNode = { + metadata: { name: 'child-1' }, + spec: { + linkId: 'scope-1', + linkType: 'scope', + parentName: 'parent-container', + nodeType: 'leaf', + title: 'Child 1', + }, + }; + + // Mock API responses + apiClient.fetchNodes.mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '') { + return Promise.resolve([parentNode]); + } else if (options.parent === 'parent-container') { + return Promise.resolve([childNode]); + } + return Promise.resolve([]); + }); + + apiClient.fetchScopeNode.mockImplementation((scopeNodeId: string) => { + if (scopeNodeId === 'child-1') { + return Promise.resolve(childNode); + } + return Promise.resolve(undefined); + }); + + // Apply scope with scopeNodeId and parentNodeId set + await service.changeScopes(['scope-1'], 'parent-container', 'child-1'); + + // Open the selector + await service.open(); + + // Verify the tree is expanded to the selected scope's parent + // The key fix: it should resolve path using scopeNodeId (child-1), not parentNodeId + expect(service.state.tree?.expanded).toBe(true); + expect(service.state.tree?.children?.['parent-container']?.expanded).toBe(true); + expect(service.state.tree?.children?.['parent-container']?.children?.['child-1']).toBeDefined(); + }); + + it('should load parent node children when opening to selected scope', async () => { + const parentNode: ScopeNode = { + metadata: { name: 'parent-container' }, + spec: { + linkId: '', + linkType: 'scope', + parentName: '', + nodeType: 'container', + title: 'Parent Container', + }, + }; + + const childNode1: ScopeNode = { + metadata: { name: 'child-1' }, + spec: { + linkId: 'scope-1', + linkType: 'scope', + parentName: 'parent-container', + nodeType: 'leaf', + title: 'Child 1', + }, + }; + + const childNode2: ScopeNode = { + metadata: { name: 'child-2' }, + spec: { + linkId: 'scope-2', + linkType: 'scope', + parentName: 'parent-container', + nodeType: 'leaf', + title: 'Child 2', + }, + }; + + const childNode3: ScopeNode = { + metadata: { name: 'child-3' }, + spec: { + linkId: 'scope-3', + linkType: 'scope', + parentName: 'parent-container', + nodeType: 'leaf', + title: 'Child 3', + }, + }; + + // Mock API responses + apiClient.fetchNodes.mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '') { + return Promise.resolve([parentNode]); + } else if (options.parent === 'parent-container') { + return Promise.resolve([childNode1, childNode2, childNode3]); + } + return Promise.resolve([]); + }); + + apiClient.fetchScopeNode.mockImplementation((scopeNodeId: string) => { + if (scopeNodeId === 'child-2') { + return Promise.resolve(childNode2); + } else if (scopeNodeId === 'parent-container') { + return Promise.resolve(parentNode); + } + return Promise.resolve(undefined); + }); + + await service.changeScopes(['scope-2'], 'parent-container', 'child-2'); + await service.open(); + + // Verify all sibling nodes are loaded (not just the selected one) + expect(service.state.tree?.children?.['parent-container']?.children?.['child-1']).toBeDefined(); + expect(service.state.tree?.children?.['parent-container']?.children?.['child-2']).toBeDefined(); + expect(service.state.tree?.children?.['parent-container']?.children?.['child-3']).toBeDefined(); + + // Verify childrenLoaded flag is set on the parent + expect(service.state.tree?.children?.['parent-container']?.childrenLoaded).toBe(true); + }); + + it('should only load children if childrenLoaded is false', async () => { + const parentNode: ScopeNode = { + metadata: { name: 'parent-container' }, + spec: { + linkId: '', + linkType: 'scope', + parentName: '', + nodeType: 'container', + title: 'Parent Container', + }, + }; + + const childNode: ScopeNode = { + metadata: { name: 'child-1' }, + spec: { + linkId: 'scope-1', + linkType: 'scope', + parentName: 'parent-container', + nodeType: 'leaf', + title: 'Child 1', + }, + }; + + apiClient.fetchNodes.mockImplementation((options: { parent?: string; query?: string; limit?: number }) => { + if (options.parent === '') { + return Promise.resolve([parentNode]); + } else if (options.parent === 'parent-container') { + return Promise.resolve([childNode]); + } + return Promise.resolve([]); + }); + + apiClient.fetchScopeNode.mockImplementation((scopeNodeId: string) => { + if (scopeNodeId === 'child-1') { + return Promise.resolve(childNode); + } else if (scopeNodeId === 'parent-container') { + return Promise.resolve(parentNode); + } + return Promise.resolve(undefined); + }); + + await service.changeScopes(['scope-1'], 'parent-container', 'child-1'); + + // First open + await service.open(); + + // Close and open again + service.closeAndReset(); + await service.open(); + + // The key: childrenLoaded flag should prevent redundant fetches + // Verify the flag is set correctly + expect(service.state.tree?.children?.['parent-container']?.childrenLoaded).toBe(true); + }); }); describe('closeAndReset', () => { diff --git a/public/app/features/scopes/selector/ScopesSelectorService.ts b/public/app/features/scopes/selector/ScopesSelectorService.ts index ede86a50135..17f36639f59 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.ts @@ -218,6 +218,8 @@ export class ScopesSelectorService extends ScopesServiceBase 0) { const fetchedScopes = await this.apiClient.fetchMultipleScopes(scopes.map((s) => s.scopeId)); + + // Fetch the scope node if it is not available + let newNodesState = { ...this.state.nodes }; + let scopeNode = scopes[0]?.scopeNodeId ? this.state.nodes[scopes[0]?.scopeNodeId] : undefined; + + if (!scopeNode && config.featureToggles.useScopeSingleNodeEndpoint && scopes[0]?.scopeNodeId) { + scopeNode = await this.apiClient.fetchScopeNode(scopes[0]?.scopeNodeId); + if (scopeNode) { + newNodesState[scopeNode.metadata.name] = scopeNode; + } + } + const newScopesState = { ...this.state.scopes }; for (const scope of fetchedScopes) { newScopesState[scope.metadata.name] = scope; } - const scopeNode = scopes[0]?.scopeNodeId ? this.state.nodes[scopes[0]?.scopeNodeId] : undefined; - - // If parentNodeId is provided, use it directly as the parent node // If not provided, try to get the parent from the scope node // When selected from recent scopes, we don't have access to the scope node (if it hasn't been loaded), but we do have access to the parent node from local storage. - const parentNodeId = scopes[0]?.parentNodeId || scopeNode?.spec.parentName; + const parentNodeId = scopes[0]?.parentNodeId ?? scopeNode?.spec.parentName; const parentNode = parentNodeId ? this.state.nodes[parentNodeId] : undefined; - this.addRecentScopes(fetchedScopes, parentNode); + this.addRecentScopes(fetchedScopes, parentNode, scopes[0]?.scopeNodeId); this.updateState({ scopes: newScopesState, loading: false }); } }; @@ -398,16 +408,19 @@ export class ScopesSelectorService extends ScopesServiceBase { + private addRecentScopes = (scopes: Scope[], parentNode?: ScopeNode, scopeNodeId?: string) => { if (scopes.length === 0) { return; } const newScopes: RecentScope[] = structuredClone(scopes); - // Set parent node for the first scope. We don't currently support multiple parent nodes being displayed, hence we only add for the first one + // Set parent node and scopeNodeId for the first scope. We don't currently support multiple parent nodes being displayed, hence we only add for the first one if (parentNode) { newScopes[0].parentNode = parentNode; } + if (scopeNodeId) { + newScopes[0].scopeNodeId = scopeNodeId; + } const RECENT_SCOPES_MAX_LENGTH = 5; @@ -452,15 +465,31 @@ export class ScopesSelectorService extends ScopesServiceBase { - if (!this.state.tree.children || Object.keys(this.state.tree.children).length === 0) { + if ( + !this.state.tree.children || + Object.keys(this.state.tree.children).length === 0 || + !this.state.tree.childrenLoaded + ) { await this.filterNode('', ''); } + // If the scopeNode isn't avilable, fetch it and add it to the nodes cache + if ( + config.featureToggles.useScopeSingleNodeEndpoint && + this.state.selectedScopes[0]?.scopeNodeId && + !this.state.nodes[this.state.selectedScopes[0].scopeNodeId] + ) { + const scopeNode = await this.apiClient.fetchScopeNode(this.state.selectedScopes[0].scopeNodeId); + if (scopeNode) { + this.updateState({ nodes: { ...this.state.nodes, [scopeNode.metadata.name]: scopeNode } }); + } + } + // First close all nodes let newTree = closeNodes(this.state.tree); - if (this.state.selectedScopes.length && this.state.selectedScopes[0].parentNodeId) { - let path = getPathOfNode(this.state.selectedScopes[0].parentNodeId, this.state.nodes); + if (this.state.selectedScopes.length && this.state.selectedScopes[0].scopeNodeId) { + let path = getPathOfNode(this.state.selectedScopes[0].scopeNodeId, this.state.nodes); // Get node at path, and request it's children if they don't exist yet let nodeAtPath = treeNodeAtPath(newTree, path); @@ -468,22 +497,30 @@ export class ScopesSelectorService extends ScopesServiceBase n.metadata.name); + path.unshift(''); nodeAtPath = treeNodeAtPath(newTree, path); } catch (error) { console.error('Failed to resolve path to root', error); } } - if (nodeAtPath && !nodeAtPath.children) { + // We have resolved to root, which means the parent node should be available + let parentPath = path.slice(0, -1); + let parentNodeAtPath = treeNodeAtPath(newTree, parentPath); + + if (parentNodeAtPath && !parentNodeAtPath.childrenLoaded) { // This will update the tree with the children - const { newTree: newTreeWithChildren } = await this.loadNodeChildren(path, nodeAtPath, ''); + const { newTree: newTreeWithChildren } = await this.loadNodeChildren(parentPath, parentNodeAtPath, ''); newTree = newTreeWithChildren; } // Expand the nodes to the selected scope - must be done after loading children try { - newTree = expandNodes(newTree, path); + newTree = expandNodes(newTree, parentPath); } catch (error) { console.error('Failed to expand nodes', error); } diff --git a/public/app/features/scopes/selector/ScopesTree.tsx b/public/app/features/scopes/selector/ScopesTree.tsx index 928827b3d32..dd2fd61184c 100644 --- a/public/app/features/scopes/selector/ScopesTree.tsx +++ b/public/app/features/scopes/selector/ScopesTree.tsx @@ -25,7 +25,7 @@ export interface ScopesTreeProps { // Recent scopes are only shown at the root node recentScopes?: Scope[][]; - onRecentScopesSelect?: (scopeIds: string[], parentNodeId?: string) => void; + onRecentScopesSelect?: (scopeIds: string[], parentNodeId?: string, scopeNodeId?: string) => void; toggleExpandedNode: (scopeNodeId: string) => void; } diff --git a/public/app/features/scopes/selector/scopesTreeUtils.test.ts b/public/app/features/scopes/selector/scopesTreeUtils.test.ts index 84f6578efbb..ca96868d5c0 100644 --- a/public/app/features/scopes/selector/scopesTreeUtils.test.ts +++ b/public/app/features/scopes/selector/scopesTreeUtils.test.ts @@ -237,5 +237,120 @@ describe('scopesTreeUtils', () => { expect(newTree.children?.child1.expanded).toBe(false); expect(newTree.children?.child1.children?.grandchild1.expanded).toBe(false); }); + + it('should set childrenLoaded to false for newly inserted nodes', () => { + const tree: TreeNode = { + expanded: false, + scopeNodeId: 'root', + query: '', + children: {}, + }; + + const path: ScopeNode[] = [ + { + metadata: { name: 'child1' }, + spec: { + parentName: 'root', + nodeType: 'container', + title: 'Child 1', + }, + }, + { + metadata: { name: 'grandchild1' }, + spec: { + parentName: 'child1', + nodeType: 'container', + title: 'Grandchild 1', + }, + }, + ]; + + const newTree = insertPathNodesIntoTree(tree, path); + + // Since we only handle insertion, it should never be true + expect(newTree.childrenLoaded).toBe(false); + + // Newly inserted nodes should have childrenLoaded set to false + expect(newTree.children?.child1.childrenLoaded).toBe(false); + expect(newTree.children?.child1.children?.grandchild1.childrenLoaded).toBe(false); + }); + + it('should preserve existing children when inserting path', () => { + const tree: TreeNode = { + expanded: true, + scopeNodeId: 'root', + query: '', + childrenLoaded: true, + children: { + existingChild: { + expanded: false, + scopeNodeId: 'existingChild', + query: '', + childrenLoaded: true, + }, + }, + }; + + const path: ScopeNode[] = [ + { + metadata: { name: 'newChild' }, + spec: { + parentName: 'root', + nodeType: 'container', + title: 'New Child', + }, + }, + ]; + + const newTree = insertPathNodesIntoTree(tree, path); + + // Existing child should still be there + expect(newTree.children?.existingChild).toBeDefined(); + expect(newTree.children?.existingChild.childrenLoaded).toBe(true); + + // New child should be added + expect(newTree.children?.newChild).toBeDefined(); + expect(newTree.children?.newChild.childrenLoaded).toBe(false); + + // Since we only handle insertion, it should never be true + expect(newTree.childrenLoaded).toBe(true); + }); + + it('should handle empty path', () => { + const tree: TreeNode = { + expanded: false, + scopeNodeId: 'root', + query: '', + children: {}, + }; + + const path: ScopeNode[] = []; + + const newTree = insertPathNodesIntoTree(tree, path); + + // Tree should remain unchanged except for childrenLoaded + expect(newTree.scopeNodeId).toBe('root'); + expect(newTree.childrenLoaded).toBeUndefined(); + }); + + it('should maintain the childrenLoaded value of the root node when inserting path', () => { + const tree: TreeNode = { + expanded: false, + scopeNodeId: 'root', + query: '', + childrenLoaded: true, + }; + + const path: ScopeNode[] = [ + { + metadata: { name: 'child1' }, + spec: { parentName: 'root', nodeType: 'container', title: 'Child 1' }, + }, + ]; + + const newTree = insertPathNodesIntoTree(tree, path); + + expect(newTree.childrenLoaded).toBe(true); + }); }); }); diff --git a/public/app/features/scopes/selector/scopesTreeUtils.ts b/public/app/features/scopes/selector/scopesTreeUtils.ts index d9184889170..8824742505d 100644 --- a/public/app/features/scopes/selector/scopesTreeUtils.ts +++ b/public/app/features/scopes/selector/scopesTreeUtils.ts @@ -126,6 +126,7 @@ export const insertPathNodesIntoTree = (tree: TreeNode, path: ScopeNode[]) => { treeNode.children = { ...treeNode.children }; if (!childNodeName) { console.warn('Failed to insert full path into tree. Did not find child to' + stringPath[index]); + treeNode.childrenLoaded = treeNode.childrenLoaded ?? false; return treeNode; } treeNode.children[childNodeName] = { @@ -133,7 +134,9 @@ export const insertPathNodesIntoTree = (tree: TreeNode, path: ScopeNode[]) => { scopeNodeId: childNodeName, query: '', children: undefined, + childrenLoaded: false, }; + treeNode.childrenLoaded = treeNode.childrenLoaded ?? false; return treeNode; }); } diff --git a/public/app/features/scopes/selector/types.ts b/public/app/features/scopes/selector/types.ts index 8a999bc98fb..b5a47b8a4e1 100644 --- a/public/app/features/scopes/selector/types.ts +++ b/public/app/features/scopes/selector/types.ts @@ -8,7 +8,7 @@ export type ScopesMap = Record; export interface SelectedScope { scopeId: string; scopeNodeId?: string; - // @deprecated Used to display title next to selected scope. scopeNodeId is used to resolve this anyways. Remove if we can confirm it doesn't break anything. + // Used for recent scopes functionality when scope node isn't loaded yet parentNodeId?: string; } @@ -17,10 +17,13 @@ export interface TreeNode { expanded: boolean; query: string; children?: Record; + // Check if we have loaded all the children. Used when resolving to root. + childrenLoaded?: boolean; } export interface RecentScope extends Scope { parentNode?: ScopeNode; + scopeNodeId?: string; } // Zod schemas for type validation @@ -64,4 +67,5 @@ export const ScopeNodeSchema = z.object({ export const RecentScopeSchema = ScopeSchema.extend({ parentNode: ScopeNodeSchema.optional(), + scopeNodeId: z.string().optional(), }); diff --git a/public/app/features/scopes/selector/useScopesHighlighting.tsx b/public/app/features/scopes/selector/useScopesHighlighting.tsx index c0b8aef8002..6aac9bddb22 100644 --- a/public/app/features/scopes/selector/useScopesHighlighting.tsx +++ b/public/app/features/scopes/selector/useScopesHighlighting.tsx @@ -62,7 +62,7 @@ export function useScopesHighlighting({ : undefined; if (parentNode?.spec.disableMultiSelect && changeScopes && scopeNodes[nodeId]?.spec.linkId) { - changeScopes([scopeNodes[nodeId].spec.linkId], parentNode.metadata.name); + changeScopes([scopeNodes[nodeId].spec.linkId], undefined, nodeId); return; } diff --git a/public/app/features/scopes/tests/selector.test.ts b/public/app/features/scopes/tests/selector.test.ts index b7083a7fc13..f12f848657a 100644 --- a/public/app/features/scopes/tests/selector.test.ts +++ b/public/app/features/scopes/tests/selector.test.ts @@ -91,7 +91,7 @@ describe('Selector', () => { it('Should initializae values from the URL', async () => { const mockLocation = { pathname: '/dashboard', - search: '?scopes=grafana&scope_parent=applications', + search: '?scopes=grafana&scope_node=applications-grafana', hash: '', key: 'test', state: null, @@ -105,6 +105,7 @@ describe('Selector', () => { // Lowercase because we don't have any backend that returns the correct case, then it falls back to the value in the URL expectScopesSelectorValue('grafana'); await openSelector(); + //screen.debug(undefined, 100000); expectResultApplicationsGrafanaSelected(); jest.spyOn(locationService, 'getLocation').mockRestore(); diff --git a/public/app/features/scopes/tests/tree.test.ts b/public/app/features/scopes/tests/tree.test.ts index f0f46e2dd9c..f11ec276c0c 100644 --- a/public/app/features/scopes/tests/tree.test.ts +++ b/public/app/features/scopes/tests/tree.test.ts @@ -194,6 +194,45 @@ describe('Tree', () => { expectResultApplicationsMimirPresent(); }); + it('Opens to a selected scope and shows all sibling nodes', async () => { + // Select a scope and apply + await openSelector(); + await expandResultApplications(); + await selectResultApplicationsMimir(); + await applyScopes(); + + // Reopen selector - should show the selected scope AND all its siblings + await openSelector(); + + // Verify all sibling nodes (Grafana, Mimir, Cloud) are visible + expectResultApplicationsGrafanaPresent(); + expectResultApplicationsMimirPresent(); + expectResultApplicationsCloudPresent(); + + // Verify the Applications container is expanded + expect(screen.getByRole('button', { name: 'Applications' })).toBeInTheDocument(); + }); + + it('Opens to a nested selected scope and shows all siblings at that level', async () => { + // Select a nested scope + await openSelector(); + await expandResultApplications(); + await expandResultApplicationsCloud(); + await selectResultApplicationsCloudDev(); + await applyScopes(); + + // Reopen selector - should expand to Cloud and show all its children + await openSelector(); + + // Verify the full path is expanded + expect(screen.getByRole('button', { name: 'Cloud' })).toBeInTheDocument(); + + // Verify all siblings at the Cloud level are visible + // The test should verify that when Cloud is expanded, we see all its children + // (This depends on what siblings Dev has - at minimum, we should see Dev itself) + expect(screen.getByRole('treeitem', { name: 'Dev' })).toBeInTheDocument(); + }); + it('Persists a scope', async () => { await openSelector(); await expandResultApplications(); @@ -291,7 +330,7 @@ describe('Tree', () => { expectScopesHeadline('Recommended'); }); - it('Should open to a specific path when scopes and scope_parent are provided', async () => { + it('Should open to a specific path when scopes and scope_node are applied', async () => { await openSelector(); await expandResultApplications(); await expandResultApplicationsCloud(); diff --git a/public/app/features/scopes/tests/utils/mocks.ts b/public/app/features/scopes/tests/utils/mocks.ts index 0a0e85b1791..c1afb8b0de2 100644 --- a/public/app/features/scopes/tests/utils/mocks.ts +++ b/public/app/features/scopes/tests/utils/mocks.ts @@ -412,6 +412,12 @@ export const getMock = jest return mocksScopes.find((scope) => scope.metadata.name.toLowerCase() === name.toLowerCase()) ?? {}; } + if (url.startsWith('/apis/scope.grafana.app/v0alpha1/namespaces/default/scopenodes/')) { + const name = url.replace('/apis/scope.grafana.app/v0alpha1/namespaces/default/scopenodes/', ''); + + return mocksNodes.find((node) => node.metadata.name === name); + } + if (url.startsWith('/apis/scope.grafana.app/v0alpha1/namespaces/default/find/scope_dashboard_bindings')) { return { items: mocksScopeDashboardBindings.filter(({ spec: { scope: bindingScope } }) =>