diff --git a/public/app/features/scopes/ScopesService.test.ts b/public/app/features/scopes/ScopesService.test.ts new file mode 100644 index 00000000000..56213b82d29 --- /dev/null +++ b/public/app/features/scopes/ScopesService.test.ts @@ -0,0 +1,323 @@ +import { BehaviorSubject } from 'rxjs'; + +import { LocationService } from '@grafana/runtime'; + +import { ScopesService } from './ScopesService'; +import { ScopesDashboardsService } from './dashboards/ScopesDashboardsService'; +import { ScopesSelectorService } from './selector/ScopesSelectorService'; + +jest.mock('./selector/ScopesSelectorService'); +jest.mock('./dashboards/ScopesDashboardsService'); + +describe('ScopesService', () => { + let service: ScopesService; + let selectorService: jest.Mocked; + let dashboardsService: jest.Mocked; + let locationService: jest.Mocked; + let stateSubscription: + | (( + state: { appliedScopes: Array<{ scopeId: string; scopeNodeId?: string; parentNodeId?: string }> }, + prevState: { appliedScopes: Array<{ scopeId: string; scopeNodeId?: string; parentNodeId?: string }> } + ) => void) + | undefined; + + beforeEach(() => { + stateSubscription = undefined; + + selectorService = { + state: { + appliedScopes: [], + selectedScopes: [], + scopes: {}, + nodes: {}, + loading: false, + opened: false, + loadingNodeName: undefined, + tree: { scopeNodeId: '', expanded: false, query: '', children: {} }, + }, + stateObservable: new BehaviorSubject({ + appliedScopes: [], + selectedScopes: [], + scopes: {}, + nodes: {}, + loading: false, + opened: false, + loadingNodeName: undefined, + tree: { scopeNodeId: '', expanded: false, query: '', children: {} }, + }), + subscribeToState: jest.fn((callback) => { + stateSubscription = callback; + return { unsubscribe: jest.fn() }; + }), + changeScopes: jest.fn(), + resolvePathToRoot: jest.fn().mockResolvedValue({ path: [], tree: {} }), + } as unknown as jest.Mocked; + + dashboardsService = { + state: { + drawerOpened: false, + dashboards: [], + scopeNavigations: [], + filteredFolders: {}, + folders: {}, + forScopeNames: [], + loading: false, + searchQuery: '', + }, + stateObservable: new BehaviorSubject({ + drawerOpened: false, + dashboards: [], + scopeNavigations: [], + filteredFolders: {}, + folders: {}, + forScopeNames: [], + loading: false, + searchQuery: '', + }), + } as unknown as jest.Mocked; + + locationService = { + getLocation: jest.fn().mockReturnValue({ + pathname: '/test', + search: '', + }), + getLocationObservable: jest.fn().mockReturnValue( + new BehaviorSubject({ + pathname: '/test', + search: '', + }) + ), + partial: jest.fn(), + } as unknown as jest.Mocked; + }); + + describe('URL initialization', () => { + it('should read scope_node from URL on init', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scope_node=node1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], undefined, 'node1'); + }); + + it('should read scope_parent for backward compatibility', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scope_parent=parent1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], 'parent1', undefined); + }); + + it('should prefer scope_node when both scope_node and scope_parent exist', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scope_node=node1&scope_parent=parent1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + // Should call with parent1 as parentNodeId and node1 as scopeNodeId + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1'], 'parent1', 'node1'); + // Should preload node1 (not parent1) + expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('node1', expect.anything()); + }); + + it('should preload scope_node when provided', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scope_node=node1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('node1', expect.anything()); + }); + + it('should fallback to preload scope_parent when scope_node is not provided', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scope_parent=parent1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + expect(selectorService.resolvePathToRoot).toHaveBeenCalledWith('parent1', expect.anything()); + }); + + it('should handle multiple scopes from URL', () => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '?scopes=scope1&scopes=scope2&scope_node=node1', + }); + + service = new ScopesService(selectorService, dashboardsService, locationService); + + expect(selectorService.changeScopes).toHaveBeenCalledWith(['scope1', 'scope2'], undefined, 'node1'); + }); + }); + + describe('URL synchronization', () => { + beforeEach(() => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '', + }); + service = new ScopesService(selectorService, dashboardsService, locationService); + }); + + it('should write scope_node to URL when scopes change', () => { + if (!stateSubscription) { + throw new Error('stateSubscription not set'); + } + + stateSubscription( + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node1' }], + }, + { + appliedScopes: [], + } + ); + + expect(locationService.partial).toHaveBeenCalledWith( + { + scopes: ['scope1'], + scope_node: 'node1', + scope_parent: null, + }, + true + ); + }); + + it('should reset scope_parent to null when writing URL', () => { + if (!stateSubscription) { + throw new Error('stateSubscription not set'); + } + + stateSubscription( + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node1', parentNodeId: 'parent1' }], + }, + { + appliedScopes: [], + } + ); + + expect(locationService.partial).toHaveBeenCalledWith( + expect.objectContaining({ + scope_parent: null, + }), + true + ); + }); + + it('should handle scopeNodeId changes without scope changes', () => { + if (!stateSubscription) { + throw new Error('stateSubscription not set'); + } + + stateSubscription( + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node2' }], + }, + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node1' }], + } + ); + + expect(locationService.partial).toHaveBeenCalledWith( + { + scopes: ['scope1'], + scope_node: 'node2', + scope_parent: null, + }, + true + ); + }); + + it('should handle missing scopeNodeId gracefully', () => { + if (!stateSubscription) { + throw new Error('stateSubscription not set'); + } + + stateSubscription( + { + appliedScopes: [{ scopeId: 'scope1' }], + }, + { + appliedScopes: [], + } + ); + + expect(locationService.partial).toHaveBeenCalledWith( + { + scopes: ['scope1'], + scope_node: null, + scope_parent: null, + }, + true + ); + }); + + it('should not update URL when scopes and scopeNodeId have not changed', () => { + if (!stateSubscription) { + throw new Error('stateSubscription not set'); + } + + jest.clearAllMocks(); + + stateSubscription( + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node1' }], + }, + { + appliedScopes: [{ scopeId: 'scope1', scopeNodeId: 'node1' }], + } + ); + + expect(locationService.partial).not.toHaveBeenCalled(); + }); + }); + + describe('setEnabled', () => { + beforeEach(() => { + locationService.getLocation = jest.fn().mockReturnValue({ + pathname: '/test', + search: '', + }); + service = new ScopesService(selectorService, dashboardsService, locationService); + }); + + it('should sync scopeNodeId when enabling scopes', () => { + selectorService.state.appliedScopes = [{ scopeId: 'scope1', scopeNodeId: 'node1' }]; + + service.setEnabled(true); + + expect(locationService.partial).toHaveBeenCalledWith( + expect.objectContaining({ + scope_node: 'node1', + }), + true + ); + }); + + it('should reset scope_parent when enabling scopes', () => { + selectorService.state.appliedScopes = [{ scopeId: 'scope1', scopeNodeId: 'node1' }]; + + service.setEnabled(true); + + expect(locationService.partial).toHaveBeenCalledWith( + expect.objectContaining({ + scope_parent: null, + }), + true + ); + }); + }); +}); diff --git a/public/app/features/scopes/ScopesService.ts b/public/app/features/scopes/ScopesService.ts index 0e6f3500839..a6d2ffed19a 100644 --- a/public/app/features/scopes/ScopesService.ts +++ b/public/app/features/scopes/ScopesService.ts @@ -70,14 +70,17 @@ 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'); - this.changeScopes(queryParams.getAll('scopes'), parentNodeId ?? undefined); + this.changeScopes(queryParams.getAll('scopes'), parentNodeId ?? undefined, scopeNodeId ?? undefined); - // Pre-load parent node, to prevent UI flickering - if (parentNodeId) { - this.selectorService.resolvePathToRoot(parentNodeId, this.selectorService.state.tree!).catch((error) => { - console.error('Failed to pre-load parent node path', error); + // Pre-load scope node (which loads parent too) or fallback to parent node for old URLs + const nodeToPreload = scopeNodeId ?? parentNodeId; + if (nodeToPreload) { + this.selectorService.resolvePathToRoot(nodeToPreload, this.selectorService.state.tree!).catch((error) => { + console.error('Failed to pre-load node path', error); }); } @@ -90,9 +93,10 @@ export class ScopesService implements ScopesContextValue { } const queryParams = new URLSearchParams(location.search); - // If we have a parent node in the URL, fetch and expand it - const parentNode = queryParams.get('scope_parent'); 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); @@ -100,7 +104,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, parentNode ?? undefined); + this.changeScopes(scopes, parentNodeId ?? undefined, scopeNodeId ?? undefined); } }) ); @@ -108,18 +112,22 @@ export class ScopesService implements ScopesContextValue { // Update the URL based on change in the scopes state this.subscriptions.push( selectorService.subscribeToState((state, prevState) => { - const oldParentNode = prevState.appliedScopes[0]?.parentNodeId; - const newParentNode = state.appliedScopes[0]?.parentNodeId; - - const parentNodeChanged = oldParentNode !== newParentNode; + const oldScopeNodeId = prevState.appliedScopes[0]?.scopeNodeId; + const newScopeNodeId = state.appliedScopes[0]?.scopeNodeId; const oldScopeNames = prevState.appliedScopes.map((scope) => scope.scopeId); const newScopeNames = state.appliedScopes.map((scope) => scope.scopeId); const scopesChanged = !isEqual(oldScopeNames, newScopeNames); - if (scopesChanged) { + const scopeNodeChanged = oldScopeNodeId !== newScopeNodeId; + + if (scopesChanged || scopeNodeChanged) { this.locationService.partial( - { scopes: newScopeNames, scope_parent: parentNodeChanged ? newParentNode || null : oldParentNode }, + { + scopes: newScopeNames, + scope_node: newScopeNodeId || null, + scope_parent: null, + }, true ); } @@ -148,8 +156,8 @@ export class ScopesService implements ScopesContextValue { return this._stateObservable; } - public changeScopes = (scopeNames: string[], parentNodeId?: string) => - this.selectorService.changeScopes(scopeNames, parentNodeId); + public changeScopes = (scopeNames: string[], parentNodeId?: string, scopeNodeId?: string) => + this.selectorService.changeScopes(scopeNames, parentNodeId, scopeNodeId); public setReadOnly = (readOnly: boolean) => { if (this.state.readOnly !== readOnly) { @@ -165,9 +173,12 @@ export class ScopesService implements ScopesContextValue { if (this.state.enabled !== enabled) { this.updateState({ enabled }); if (enabled) { + const scopeNodeId = this.selectorService.state.appliedScopes[0]?.scopeNodeId; this.locationService.partial( { scopes: this.selectorService.state.appliedScopes.map((s) => s.scopeId), + scope_node: scopeNodeId, + scope_parent: null, }, true ); diff --git a/public/app/features/scopes/selector/ScopesInput.tsx b/public/app/features/scopes/selector/ScopesInput.tsx index 6aed84a83e9..39b9edd8295 100644 --- a/public/app/features/scopes/selector/ScopesInput.tsx +++ b/public/app/features/scopes/selector/ScopesInput.tsx @@ -34,9 +34,18 @@ export function ScopesInput({ }: ScopesInputProps) { const [tooltipVisible, setTooltipVisible] = useState(false); - const parentNodeId = appliedScopes[0]?.parentNodeId; + const scopeNodeId = appliedScopes[0]?.scopeNodeId; + const parentNodeIdFromUrl = appliedScopes[0]?.parentNodeId; + + 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; const { node: parentNode, isLoading: parentNodeLoading } = useScopeNode(parentNodeId); - const parentNodeTitle = parentNode?.spec.title; + + // Prioritize scope node subtitle over parent node title + const displayTitle = scopeNode?.spec.subTitle ?? parentNode?.spec.title; + const isLoadingTitle = scopeNodeLoading || parentNodeLoading; useEffect(() => { setTooltipVisible(false); @@ -59,12 +68,8 @@ export function ScopesInput({ const parentNodePrefix = useMemo( () => - parentNodeLoading ? ( - - ) : parentNodeTitle ? ( - {parentNodeTitle}: - ) : undefined, - [parentNodeLoading, parentNodeTitle] + isLoadingTitle ? : displayTitle ? {displayTitle}: : undefined, + [isLoadingTitle, displayTitle] ); const input = useMemo( diff --git a/public/app/features/scopes/selector/ScopesSelectorService.test.ts b/public/app/features/scopes/selector/ScopesSelectorService.test.ts index 39e444fc6a5..eef594d2335 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.test.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.test.ts @@ -316,6 +316,31 @@ describe('ScopesSelectorService', () => { expect(service.state.nodes).toEqual({ 'test-scope-node': mockNode }); expect(storeValue[RECENT_SCOPES_KEY]).toEqual(JSON.stringify([[{ ...mockScope, parentNode: mockNode }]])); }); + + it('should set scopeNodeId for the first scope only', async () => { + await service.changeScopes(['test-scope', 'test-scope-2'], 'parent-node', 'scope-node-1'); + + expect(service.state.appliedScopes).toEqual([ + { scopeId: 'test-scope', scopeNodeId: 'scope-node-1', parentNodeId: 'parent-node' }, + { scopeId: 'test-scope-2', scopeNodeId: undefined, parentNodeId: 'parent-node' }, + ]); + }); + + it('should handle scopeNodeId without parentNodeId', async () => { + await service.changeScopes(['test-scope'], undefined, 'scope-node-1'); + + expect(service.state.appliedScopes).toEqual([ + { scopeId: 'test-scope', scopeNodeId: 'scope-node-1', parentNodeId: undefined }, + ]); + }); + + it('should maintain backward compatibility when only parentNodeId is provided', async () => { + await service.changeScopes(['test-scope'], 'parent-node'); + + expect(service.state.appliedScopes).toEqual([ + { scopeId: 'test-scope', scopeNodeId: undefined, parentNodeId: 'parent-node' }, + ]); + }); }); describe('open', () => { diff --git a/public/app/features/scopes/selector/ScopesSelectorService.ts b/public/app/features/scopes/selector/ScopesSelectorService.ts index b7c502de5bd..598e0de7757 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.ts @@ -341,8 +341,15 @@ export class ScopesSelectorService extends ScopesServiceBase { - return this.applyScopes(scopeNames.map((id) => ({ scopeId: id, parentNodeId }))); + changeScopes = (scopeNames: string[], parentNodeId?: string, scopeNodeId?: string) => { + return this.applyScopes( + scopeNames.map((id, index) => ({ + scopeId: id, + // Only the first scope gets the scopeNodeId + scopeNodeId: index === 0 ? scopeNodeId : undefined, + parentNodeId, + })) + ); }; /** diff --git a/public/app/features/scopes/selector/types.ts b/public/app/features/scopes/selector/types.ts index f4eb0a4c876..8a999bc98fb 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; - // Used to display title next to selected scope + // @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. parentNodeId?: string; } @@ -47,6 +47,7 @@ export const ScopeSchema = z.object({ export const ScopeNodeSpecSchema = z.object({ nodeType: z.enum(['container', 'leaf']), title: z.string(), + subTitle: z.string().optional(), description: z.string().optional(), disableMultiSelect: z.boolean().optional(), linkId: z.string().optional(),