From 3048fc620592703d9224b66d52938ee60816581b Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Tue, 5 Aug 2025 14:09:58 +0200 Subject: [PATCH] Scopes: Save parent node with recent scope (#108795) * Read and write recent scopes * Store entire ScopeNode, load from localSotrage * Load nodes form localStorage on init * Add runtime type validation * Add tests * Remove unused import * Add parentNode name to selector integration test * Deep clone * Schema validation with zod * Add comment * Use zod for schema validation of scope object * Update type validation of recent scope nodes * Add comment * Add expect ts error * Fix nit --- .../actions/recentScopesActions.ts | 2 + .../features/scopes/selector/RecentScopes.tsx | 26 ++- .../scopes/selector/ScopesSelector.tsx | 4 +- .../selector/ScopesSelectorService.test.ts | 192 +++++++++++++++++- .../scopes/selector/ScopesSelectorService.ts | 74 +++++-- .../features/scopes/selector/ScopesTree.tsx | 2 +- public/app/features/scopes/selector/types.ts | 50 +++++ .../features/scopes/tests/selector.test.ts | 20 +- 8 files changed, 327 insertions(+), 43 deletions(-) diff --git a/public/app/features/commandPalette/actions/recentScopesActions.ts b/public/app/features/commandPalette/actions/recentScopesActions.ts index 8421e2ea4bf..5afbf3fd115 100644 --- a/public/app/features/commandPalette/actions/recentScopesActions.ts +++ b/public/app/features/commandPalette/actions/recentScopesActions.ts @@ -20,6 +20,8 @@ export function getRecentScopesActions(): CommandPaletteAction[] { id: recentScope.map((scope) => scope.spec.title).join(', '), name: recentScope.map((scope) => scope.spec.title).join(', '), section: t('command-palette.section.recent-scopes', 'Recent scopes'), + // Only show the parent of the first scope for now + subtitle: recentScope[0]?.parentNode?.spec.title, priority: RECENT_SCOPES_PRIORITY, perform: () => { scopesSelectorService.changeScopes(recentScope.map((scope) => scope.metadata.name)); diff --git a/public/app/features/scopes/selector/RecentScopes.tsx b/public/app/features/scopes/selector/RecentScopes.tsx index da7d2070954..efd190f7ec3 100644 --- a/public/app/features/scopes/selector/RecentScopes.tsx +++ b/public/app/features/scopes/selector/RecentScopes.tsx @@ -1,13 +1,15 @@ import { css } from '@emotion/css'; import { useId, useState } from 'react'; -import { GrafanaTheme2, Scope } from '@grafana/data'; +import { GrafanaTheme2 } from '@grafana/data'; import { Trans } from '@grafana/i18n'; import { useStyles2, Stack, Text, Icon, Box } from '@grafana/ui'; +import { RecentScope } from './types'; + interface RecentScopesProps { - recentScopes: Scope[][]; - onSelect: (scopeIds: string[]) => void; + recentScopes: RecentScope[][]; + onSelect: (scopeIds: string[], parentNodeId?: string) => void; } export const RecentScopes = ({ recentScopes, onSelect }: RecentScopesProps) => { @@ -39,10 +41,18 @@ export const RecentScopes = ({ recentScopes, onSelect }: RecentScopesProps) => { className={styles.recentScopeButton} key={recentScopeSet.map((s) => s.metadata.name).join(',')} onClick={() => { - onSelect(recentScopeSet.map((s) => s.metadata.name)); + onSelect( + recentScopeSet.map((s) => s.metadata.name), + recentScopeSet[0]?.parentNode?.metadata?.name + ); }} > - {recentScopeSet.map((s) => s.spec.title).join(', ')} + {recentScopeSet.map((s) => s.spec.title).join(', ')} + {recentScopeSet[0]?.parentNode?.spec.title && ( + + {recentScopeSet[0]?.parentNode?.spec.title} + + )} ))} @@ -58,9 +68,9 @@ const getStyles = (theme: GrafanaTheme2) => ({ border: 'none', padding: 0, cursor: 'pointer', - textOverflow: 'ellipsis', - overflow: 'hidden', - whiteSpace: 'nowrap', + display: 'flex', + gap: theme.spacing(1), + alignItems: 'center', }), expandButton: css({ display: 'flex', diff --git a/public/app/features/scopes/selector/ScopesSelector.tsx b/public/app/features/scopes/selector/ScopesSelector.tsx index 5b0fed1bfc1..5a40106070e 100644 --- a/public/app/features/scopes/selector/ScopesSelector.tsx +++ b/public/app/features/scopes/selector/ScopesSelector.tsx @@ -105,8 +105,8 @@ export const ScopesSelector = () => { scopeNodes={nodes} selectScope={selectScope} deselectScope={deselectScope} - onRecentScopesSelect={(scopeIds: string[]) => { - scopesSelectorService.changeScopes(scopeIds); + onRecentScopesSelect={(scopeIds: string[], parentNodeId?: string) => { + scopesSelectorService.changeScopes(scopeIds, parentNodeId); scopesSelectorService.closeAndReset(); }} /> diff --git a/public/app/features/scopes/selector/ScopesSelectorService.test.ts b/public/app/features/scopes/selector/ScopesSelectorService.test.ts index bb9ab1c40c4..f1e7265e735 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.test.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.test.ts @@ -4,6 +4,7 @@ import { ScopesApiClient } from '../ScopesApiClient'; import { ScopesDashboardsService } from '../dashboards/ScopesDashboardsService'; import { RECENT_SCOPES_KEY, ScopesSelectorService } from './ScopesSelectorService'; +import { RecentScope } from './types'; describe('ScopesSelectorService', () => { let service: ScopesSelectorService; @@ -42,6 +43,7 @@ describe('ScopesSelectorService', () => { }; let storeValue: Record = {}; + let store: Store; beforeEach(() => { apiClient = { @@ -63,17 +65,23 @@ describe('ScopesSelectorService', () => { } as unknown as jest.Mocked; storeValue = {}; - const store = { + store = { get(key: string) { return storeValue[key]; }, - set(key: string, value: string) { storeValue[key] = value; }, - }; + subscribe: jest.fn(), + notifySubscribers: jest.fn(), + getBool: jest.fn(), + getObject: jest.fn(), + setObject: jest.fn(), + exists: jest.fn(), + delete: jest.fn(), + } as unknown as Store; - service = new ScopesSelectorService(apiClient, dashboardsService, store as Store); + service = new ScopesSelectorService(apiClient, dashboardsService, store); }); describe('updateNode', () => { @@ -125,6 +133,10 @@ describe('ScopesSelectorService', () => { await service.deselectScope('test-scope-node'); expect(service.state.selectedScopes).toEqual([]); }); + + it('should set recent scopes', async () => { + await service.selectScope('test-scope-node'); + }); }); describe('changeScopes', () => { @@ -154,6 +166,16 @@ describe('ScopesSelectorService', () => { sub.unsubscribe(); }); + + it('should set parent node for recent scopes', async () => { + // Load mock node + await service.updateNode('', true, ''); + + await service.changeScopes(['test-scope'], 'test-scope-node'); + expect(service.state.appliedScopes).toEqual([{ scopeId: 'test-scope', parentNodeId: 'test-scope-node' }]); + expect(service.state.nodes).toEqual({ 'test-scope-node': mockNode }); + expect(storeValue[RECENT_SCOPES_KEY]).toEqual(JSON.stringify([[{ ...mockScope, parentNode: mockNode }]])); + }); }); describe('open', () => { @@ -253,4 +275,166 @@ describe('ScopesSelectorService', () => { expect(recentScopes).toEqual([]); }); }); + + describe('nodes from local storage', () => { + it('should return parent nodes from recent scopes', async () => { + // Set mock scopes with parent node + const mockScopeWithParentNode: RecentScope = { + metadata: { name: 'test-scope' }, + spec: { + title: 'test-scope', + type: 'scope', + category: 'scope', + description: 'test scope', + filters: [], + }, + parentNode: { + metadata: { name: 'test-scope-node' }, + spec: { + linkId: 'test-scope', + linkType: 'scope', + parentName: '', + nodeType: 'container', + title: 'test-scope-node', + }, + }, + }; + + // Set store value BEFORE creating the service + storeValue[RECENT_SCOPES_KEY] = JSON.stringify([[mockScopeWithParentNode]]); + + // Create service with the existing store (which now has the data) + service = new ScopesSelectorService(apiClient, dashboardsService, store as Store); + expect(service.state.nodes).toEqual({ 'test-scope-node': mockScopeWithParentNode.parentNode }); + }); + + it('should remove parent node if it is not valid', async () => { + // Mock with valid parent node + const mockScopeWithValidParentNode: RecentScope = { + metadata: { name: 'test-scope' }, + spec: { + title: 'test-scope', + type: 'scope', + category: 'scope', + description: 'test scope', + filters: [], + }, + parentNode: { + metadata: { name: 'test-scope-node' }, + spec: { + linkId: 'test-scope', + linkType: 'scope', + parentName: '', + nodeType: 'container', + title: 'test-scope-node', + }, + }, + }; + + // lacks name and spec + const mockScopeWithInvalidParentNode: RecentScope = { + metadata: { name: 'test-scope' }, + spec: { + title: 'test-scope', + type: 'scope', + category: 'scope', + description: 'test scope', + filters: [], + }, + parentNode: { + //@ts-expect-error + metadata: {}, + //@ts-expect-error + spec: {}, + }, + }; + + // Set store value BEFORE creating the service + storeValue[RECENT_SCOPES_KEY] = JSON.stringify([ + [mockScopeWithInvalidParentNode], + [mockScopeWithValidParentNode], + ]); + + // Create service with the existing store (which now has the data) + service = new ScopesSelectorService(apiClient, dashboardsService, store as Store); + expect(service.state.nodes).toEqual({ 'test-scope-node': mockScopeWithValidParentNode.parentNode }); + }); + + it('should validate parent nodes across all recent scope sets', async () => { + // Create multiple scope sets with various parent node validity + const mockScopeWithValidParentNode: RecentScope = { + metadata: { name: 'valid-scope' }, + spec: { + title: 'valid-scope', + type: 'scope', + category: 'scope', + description: 'valid scope', + filters: [], + }, + parentNode: { + metadata: { name: 'valid-parent-node' }, + spec: { + linkId: 'valid-scope', + linkType: 'scope', + parentName: '', + nodeType: 'container', + title: 'valid-parent-node', + }, + }, + }; + + const mockScopeWithInvalidParentNode1: RecentScope = { + metadata: { name: 'invalid-scope-1' }, + spec: { + title: 'invalid-scope-1', + type: 'scope', + category: 'scope', + description: 'invalid scope 1', + filters: [], + }, + parentNode: { + //@ts-expect-error + metadata: {}, + //@ts-expect-error + spec: {}, + }, + }; + + const mockScopeWithInvalidParentNode2: RecentScope = { + metadata: { name: 'invalid-scope-2' }, + spec: { + title: 'invalid-scope-2', + type: 'scope', + category: 'scope', + description: 'invalid scope 2', + filters: [], + }, + parentNode: { + metadata: { name: 'invalid-parent-node-2' }, // missing spec + //@ts-expect-error - intentionally invalid spec for testing + spec: {}, + }, + }; + + // Set store value with multiple scope sets - some with invalid parent nodes + storeValue[RECENT_SCOPES_KEY] = JSON.stringify([ + [mockScopeWithInvalidParentNode1], + [mockScopeWithValidParentNode], + [mockScopeWithInvalidParentNode2], + ]); + + // Create service with the existing store + service = new ScopesSelectorService(apiClient, dashboardsService, store as Store); + + // Should only include the valid parent node + expect(service.state.nodes).toEqual({ 'valid-parent-node': mockScopeWithValidParentNode.parentNode }); + + // Verify that the invalid parent nodes were removed from the stored data + const recentScopes = service.getRecentScopes(); + expect(recentScopes).toHaveLength(3); + expect(recentScopes[0][0].parentNode).toBeUndefined(); // invalid parent node should be removed + expect(recentScopes[1][0].parentNode).toEqual(mockScopeWithValidParentNode.parentNode); // valid parent node should remain + expect(recentScopes[2][0].parentNode).toBeUndefined(); // invalid parent node should be removed + }); + }); }); diff --git a/public/app/features/scopes/selector/ScopesSelectorService.ts b/public/app/features/scopes/selector/ScopesSelectorService.ts index b8daece424c..6c6493ca16d 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.ts @@ -1,4 +1,4 @@ -import { Scope, store as storeImpl } from '@grafana/data'; +import { Scope, ScopeNode, store as storeImpl } from '@grafana/data'; import { ScopesApiClient } from '../ScopesApiClient'; import { ScopesServiceBase } from '../ScopesServiceBase'; @@ -13,7 +13,7 @@ import { modifyTreeNodeAtPath, treeNodeAtPath, } from './scopesTreeUtils'; -import { NodesMap, ScopesMap, SelectedScope, TreeNode } from './types'; +import { NodesMap, RecentScope, RecentScopeSchema, ScopeSchema, ScopesMap, SelectedScope, TreeNode } from './types'; export const RECENT_SCOPES_KEY = 'grafana.scopes.recent'; @@ -69,6 +69,10 @@ export class ScopesSelectorService extends ScopesServiceBase { - return this.applyScopes(scopeNames.map((id) => ({ scopeId: id, parentNodeId: parentNodeId }))); + return this.applyScopes(scopeNames.map((id) => ({ scopeId: id, parentNodeId }))); }; /** @@ -266,22 +270,37 @@ export class ScopesSelectorService extends ScopesServiceBase this.applyScopes([]); - private addRecentScopes = (scopes: Scope[]) => { + private addRecentScopes = (scopes: Scope[], parentNode?: ScopeNode) => { 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 + if (parentNode) { + newScopes[0].parentNode = parentNode; + } + const RECENT_SCOPES_MAX_LENGTH = 5; const recentScopes = this.getRecentScopes(); - recentScopes.unshift(scopes); + recentScopes.unshift(newScopes); this.store.set(RECENT_SCOPES_KEY, JSON.stringify(recentScopes.slice(0, RECENT_SCOPES_MAX_LENGTH - 1))); }; @@ -289,12 +308,12 @@ export class ScopesSelectorService extends ScopesServiceBase { + public getRecentScopes = (): RecentScope[][] => { const content: string | undefined = this.store.get(RECENT_SCOPES_KEY); const recentScopes = parseScopesFromLocalStorage(content); // Filter out the current selection from recent scopes to avoid duplicates - return recentScopes.filter((scopes: Scope[]) => { + return recentScopes.filter((scopes: RecentScope[]) => { if (scopes.length !== this.state.appliedScopes.length) { return true; } @@ -303,6 +322,20 @@ export class ScopesSelectorService extends ScopesServiceBase => { + const content: string | undefined = this.store.get(RECENT_SCOPES_KEY); + const recentScopes = parseScopesFromLocalStorage(content); + + // Load parent nodes for recent scopes + const parentNodes = Object.fromEntries( + recentScopes + .map((scopes) => [scopes[0]?.parentNode?.metadata?.name, scopes[0]?.parentNode]) + .filter(([key, parentNode]) => parentNode !== undefined && key !== undefined) + ); + + return parentNodes; + }; + /** * Opens the scopes selector drawer and loads the root nodes if they are not loaded yet. */ @@ -361,18 +394,14 @@ function isScopeLocalStorageV1(obj: unknown): obj is { scope: Scope } { } function isScopeObj(obj: unknown): obj is Scope { - return ( - typeof obj === 'object' && - obj !== null && - 'metadata' in obj && - typeof obj['metadata'] === 'object' && - obj['metadata'] !== null && - 'name' in obj['metadata'] && - 'spec' in obj - ); + return ScopeSchema.safeParse(obj).success; } -function parseScopesFromLocalStorage(content: string | undefined): Scope[][] { +function hasValidScopeParentNode(obj: unknown): obj is RecentScope { + return RecentScopeSchema.safeParse(obj).success; +} + +function parseScopesFromLocalStorage(content: string | undefined): RecentScope[][] { let recentScopes; try { recentScopes = JSON.parse(content || '[]'); @@ -391,5 +420,14 @@ function parseScopesFromLocalStorage(content: string | undefined): Scope[][] { return []; } + // Verify the structure of the parent node for all recent scope sets, and remove it if it is not valid + for (const scopeSet of recentScopes) { + if (scopeSet[0]?.parentNode) { + if (!hasValidScopeParentNode(scopeSet[0])) { + scopeSet[0].parentNode = undefined; + } + } + } + return recentScopes; } diff --git a/public/app/features/scopes/selector/ScopesTree.tsx b/public/app/features/scopes/selector/ScopesTree.tsx index e3fbc06e203..e21ec1bc808 100644 --- a/public/app/features/scopes/selector/ScopesTree.tsx +++ b/public/app/features/scopes/selector/ScopesTree.tsx @@ -23,7 +23,7 @@ export interface ScopesTreeProps { // Recent scopes are only shown at the root node recentScopes?: Scope[][]; - onRecentScopesSelect?: (scopeIds: string[]) => void; + onRecentScopesSelect?: (scopeIds: string[], parentNodeId?: string) => void; } export function ScopesTree({ diff --git a/public/app/features/scopes/selector/types.ts b/public/app/features/scopes/selector/types.ts index f30b7e04868..b73b9e66bd0 100644 --- a/public/app/features/scopes/selector/types.ts +++ b/public/app/features/scopes/selector/types.ts @@ -1,3 +1,5 @@ +import { z } from 'zod'; + import { Scope, ScopeNode } from '@grafana/data'; export type NodesMap = Record; @@ -16,3 +18,51 @@ export interface TreeNode { query: string; children?: Record; } + +export interface RecentScope extends Scope { + parentNode?: ScopeNode; +} + +// Zod schemas for type validation +export const ScopeSpecFilterSchema = z.object({ + key: z.string(), + value: z.string(), + values: z.array(z.string()).optional(), + operator: z.enum(['equals', 'not-equals', 'regex-match', 'regex-not-match', 'one-of', 'not-one-of']), +}); + +export const ScopeSpecSchema = z.object({ + title: z.string(), + type: z.string(), + description: z.string(), + category: z.string(), + filters: z.array(ScopeSpecFilterSchema), +}); + +export const ScopeSchema = z.object({ + metadata: z.object({ + name: z.string(), + }), + spec: ScopeSpecSchema, +}); + +export const ScopeNodeSpecSchema = z.object({ + nodeType: z.enum(['container', 'leaf']), + title: z.string(), + description: z.string().optional(), + disableMultiSelect: z.boolean().optional(), + linkId: z.string().optional(), + linkType: z.enum(['scope']).optional(), + parentName: z.string().optional(), +}); + +export const ScopeNodeSchema = z.object({ + metadata: z.object({ + name: z.string(), + }), + spec: ScopeNodeSpecSchema, +}); + +export const RecentScopeSchema = ScopeSchema.extend({ + parentNode: ScopeNodeSchema.optional(), +}); diff --git a/public/app/features/scopes/tests/selector.test.ts b/public/app/features/scopes/tests/selector.test.ts index 9f9b5e55596..c996caeaded 100644 --- a/public/app/features/scopes/tests/selector.test.ts +++ b/public/app/features/scopes/tests/selector.test.ts @@ -102,18 +102,18 @@ describe('Selector', () => { await openSelector(); expectRecentScopesSection(); await expandRecentScopes(); - expectRecentScope('Grafana'); - expectRecentScope('Grafana, Mimir'); - await selectRecentScope('Grafana'); + expectRecentScope('Grafana Applications'); + expectRecentScope('Grafana, Mimir Applications'); + await selectRecentScope('Grafana Applications'); expectScopesSelectorValue('Grafana'); await openSelector(); await expandRecentScopes(); - expectRecentScope('Grafana, Mimir'); - expectRecentScopeNotPresent('Grafana'); - expectRecentScopeNotPresent('Mimir'); - await selectRecentScope('Grafana, Mimir'); + expectRecentScope('Grafana, Mimir Applications'); + expectRecentScopeNotPresent('Grafana Applications'); + expectRecentScopeNotPresent('Mimir Applications'); + await selectRecentScope('Grafana, Mimir Applications'); expectScopesSelectorValue('Grafana + Mimir'); }); @@ -148,7 +148,7 @@ describe('Selector', () => { await openSelector(); expectRecentScopesSection(); await expandRecentScopes(); - expectRecentScope('Grafana, Mimir'); + expectRecentScope('Grafana, Mimir Applications'); }); it('should update recent scopes when selecting a different combination', async () => { @@ -169,8 +169,8 @@ describe('Selector', () => { // Check recent scopes are updated await openSelector(); await expandRecentScopes(); - expectRecentScope('Grafana, Mimir'); - expectRecentScope('Grafana'); + expectRecentScope('Grafana, Mimir Applications'); + expectRecentScope('Grafana Applications'); }); }); });