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
2025-10-31 13:36:10 +01:00
parent 4813256e0a
commit f9c0924f0c
6 changed files with 399 additions and 27 deletions
@@ -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<ScopesSelectorService>;
+  let dashboardsService: jest.Mocked<ScopesDashboardsService>;
+  let locationService: jest.Mocked<LocationService>;
+  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<ScopesSelectorService>;
+
+    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<ScopesDashboardsService>;
+
+    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<LocationService>;
+  });
+
+  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
+      );
+    });
+  });
+});
@@ -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
        );
@@ -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 ? (
-        <Skeleton width={30} height={14} />
-      ) : parentNodeTitle ? (
-        <span>{parentNodeTitle}:</span>
-      ) : undefined,
-    [parentNodeLoading, parentNodeTitle]
+      isLoadingTitle ? <Skeleton width={30} height={14} /> : displayTitle ? <span>{displayTitle}:</span> : undefined,
+    [isLoadingTitle, displayTitle]
  );

  const input = useMemo(
@@ -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', () => {
@@ -341,8 +341,15 @@ export class ScopesSelectorService extends ScopesServiceBase<ScopesSelectorServi
    return this.collapseNode(scopeNodeId);
  };

-  changeScopes = (scopeNames: string[], parentNodeId?: string) => {
-    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,
+      }))
+    );
  };

  /**
@@ -8,7 +8,7 @@ export type ScopesMap = Record<string, Scope>;
 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(),