From f9c0924f0cce039cc9ea565b10b38dd75d444e05 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Fri, 31 Oct 2025 13:36:10 +0100 Subject: [PATCH] Scopes: Sync scope_node in favor of scope_parent in the URL (#113212) * Enhance ScopesService to support scopeNodeId in URL parameters for improved backward compatibility. Update changeScopes method to accept scopeNodeId, allowing for better handling of scope nodes. Adjust ScopesInput to prioritize scope node titles and ensure loading states are managed correctly. Refactor related logic in ScopesSelectorService for consistent scope handling. * Scopes: Add tests for scope_node URL sync and scopeNodeId handling - Add ScopesService.test.ts with tests for URL parameter handling - Test scope_node and scope_parent reading from URL - Test scope_node writing to URL with scope_parent reset - Test backward compatibility with legacy scope_parent - Test URL sync when scopes and scopeNodeId change - Add tests to ScopesSelectorService.test.ts for changeScopes - Test scopeNodeId assignment (only first scope gets it) - Test handling scopeNodeId without parentNodeId - Test backward compatibility when only parentNodeId provided All 13 new tests passing, maintaining 100% test coverage. * Fix linting error * Fix comments --- .../app/features/scopes/ScopesService.test.ts | 323 ++++++++++++++++++ public/app/features/scopes/ScopesService.ts | 43 ++- .../features/scopes/selector/ScopesInput.tsx | 21 +- .../selector/ScopesSelectorService.test.ts | 25 ++ .../scopes/selector/ScopesSelectorService.ts | 11 +- public/app/features/scopes/selector/types.ts | 3 +- 6 files changed, 399 insertions(+), 27 deletions(-) create mode 100644 public/app/features/scopes/ScopesService.test.ts 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(),