diff --git a/public/app/features/scopes/dashboards/ScopesDashboardsService.test.ts b/public/app/features/scopes/dashboards/ScopesDashboardsService.test.ts index 1e91d084822..15a10f43f3f 100644 --- a/public/app/features/scopes/dashboards/ScopesDashboardsService.test.ts +++ b/public/app/features/scopes/dashboards/ScopesDashboardsService.test.ts @@ -1,4 +1,5 @@ import { Location } from 'history'; +import { Subject } from 'rxjs'; import { ScopeDashboardBinding } from '@grafana/data'; import { config, locationService } from '@grafana/runtime'; @@ -17,6 +18,10 @@ jest.mock('@grafana/runtime', () => ({ }, locationService: { getLocation: jest.fn(), + // Mock getLocationObservable to return a mock observable + getLocationObservable: jest.fn().mockReturnValue({ + subscribe: jest.fn(), + }), }, })); @@ -61,12 +66,11 @@ describe('ScopesDashboardsService', () => { expect(service.state.folders[''].folders['group1'].expanded).toBe(true); }); - it('should expand folders when current location matches URL path and navigation endpoint is enabled', async () => { - // Enable the navigation endpoint feature toggle + it('should expand folder when location changes and matches a navigation URL', async () => { config.featureToggles.useScopesNavigationEndpoint = true; - // Mock current location to match a URL-based navigation - (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/test-url' } as Location); + // Mock initial location + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/' } as Location); const mockNavigations: ScopeNavigation[] = [ { @@ -85,10 +89,73 @@ describe('ScopesDashboardsService', () => { ]; mockApiClient.fetchScopeNavigations.mockResolvedValue(mockNavigations); - await service.fetchDashboards(['scope1']); - // Verify that the folder is expanded because the current URL path matches - expect(service.state.folders[''].folders['group1'].expanded).toBe(true); + // Set up mock observable to emit location changes + const locationSubject = new Subject(); + (locationService.getLocationObservable as jest.Mock).mockReturnValue(locationSubject); + + // Create a new service instance that will subscribe to our mocked observable + const testService = new ScopesDashboardsService(mockApiClient); + await testService.fetchDashboards(['scope1']); + + // Initially, folder should not be expanded since we're at '/' + expect(testService.state.folders[''].folders['group1'].expanded).toBe(false); + + // Simulate location change to a URL that matches a navigation + locationSubject.next({ pathname: '/test-url' } as Location); + + // Now the folder should be expanded because the location matches a navigation URL + expect(testService.state.folders[''].folders['group1'].expanded).toBe(true); + + // Reset the feature toggle + config.featureToggles.useScopesNavigationEndpoint = false; + }); + + it('should not expand folder when location changes, matches a navigation URL in a folder which is already expanded', async () => { + config.featureToggles.useScopesNavigationEndpoint = true; + + // Mock initial location + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/' } as Location); + + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'scope1', + url: '/test-url', + }, + status: { + title: 'Test URL', + groups: ['group1', 'group2'], + }, + metadata: { + name: 'url1', + }, + }, + ]; + + mockApiClient.fetchScopeNavigations.mockResolvedValue(mockNavigations); + + // Set up mock observable to emit location changes + const locationSubject = new Subject(); + (locationService.getLocationObservable as jest.Mock).mockReturnValue(locationSubject); + + // Create a new service instance that will subscribe to our mocked observable + const testService = new ScopesDashboardsService(mockApiClient); + await testService.fetchDashboards(['scope1']); + + // Initially, folder should not be expanded since we're at '/' + expect(testService.state.folders[''].folders['group1'].expanded).toBe(false); + + // Manually expand group1 to simulate it being already expanded + testService.updateFolder(['', 'group2'], true); + expect(testService.state.folders[''].folders['group2'].expanded).toBe(true); + + // Simulate location change to a URL that matches a navigation + locationSubject.next({ pathname: '/test-url' } as Location); + + // The folder should still be expanded (no change since it was already expanded) + expect(testService.state.folders[''].folders['group1'].expanded).toBe(false); + expect(testService.state.folders[''].folders['group2'].expanded).toBe(true); // Reset the feature toggle config.featureToggles.useScopesNavigationEndpoint = false; diff --git a/public/app/features/scopes/dashboards/ScopesDashboardsService.ts b/public/app/features/scopes/dashboards/ScopesDashboardsService.ts index 8ab1e7bff0c..42909b4663f 100644 --- a/public/app/features/scopes/dashboards/ScopesDashboardsService.ts +++ b/public/app/features/scopes/dashboards/ScopesDashboardsService.ts @@ -1,4 +1,5 @@ import { isEqual } from 'lodash'; +import { Subscription } from 'rxjs'; import { ScopeDashboardBinding } from '@grafana/data'; import { config, locationService } from '@grafana/runtime'; @@ -6,6 +7,7 @@ import { config, locationService } from '@grafana/runtime'; import { ScopesApiClient } from '../ScopesApiClient'; import { ScopesServiceBase } from '../ScopesServiceBase'; +import { isCurrentPath } from './scopeNavgiationUtils'; import { ScopeNavigation, SuggestedNavigationsFoldersMap } from './types'; interface ScopesDashboardsServiceState { @@ -24,6 +26,7 @@ interface ScopesDashboardsServiceState { } export class ScopesDashboardsService extends ScopesServiceBase { + private locationSubscription: Subscription | undefined; constructor(private apiClient: ScopesApiClient) { super({ drawerOpened: false, @@ -35,8 +38,56 @@ export class ScopesDashboardsService extends ScopesServiceBase { + if (state.drawerOpened === prevState.drawerOpened) { + return; + } + if (state.drawerOpened && !prevState.drawerOpened) { + // Before creating a new subscription, ensure any existing subscription is disposed to avoid multiple active subscriptions and potential memory leaks. + this.locationSubscription?.unsubscribe(); + this.locationSubscription = locationService.getLocationObservable().subscribe((location) => { + this.onLocationChange(location.pathname); + }); + } else if (!state.drawerOpened && prevState.drawerOpened) { + this.locationSubscription?.unsubscribe(); + } + }); } + // Expand the group that matches the current path, if it is not already expanded + private onLocationChange = (pathname: string) => { + if (!this.state.drawerOpened) { + return; + } + const currentPath = pathname; + const activeScopeNavigation = this.state.scopeNavigations.find((s) => { + if (!('url' in s.spec) || typeof s.spec.url !== 'string') { + return false; + } + return isCurrentPath(currentPath, s.spec.url); + }); + + if (!activeScopeNavigation) { + return; + } + + // Check if the activeScopeNavigation is in a folder that is already expanded + if (activeScopeNavigation.status.groups) { + for (const group of activeScopeNavigation.status.groups) { + if (this.state.folders[''].folders[group].expanded) { + return; + } + } + } + + // Expand the first group, as we don't know which one to prioritize + if (activeScopeNavigation.status.groups) { + this.updateFolder(['', activeScopeNavigation.status.groups[0]], true); + } + }; + public updateFolder = (path: string[], expanded: boolean) => { let folders = { ...this.state.folders }; let filteredFolders = { ...this.state.filteredFolders }; diff --git a/public/app/features/scopes/dashboards/ScopesNavigationTreeLink.tsx b/public/app/features/scopes/dashboards/ScopesNavigationTreeLink.tsx index 589cbe9c743..41481ca0446 100644 --- a/public/app/features/scopes/dashboards/ScopesNavigationTreeLink.tsx +++ b/public/app/features/scopes/dashboards/ScopesNavigationTreeLink.tsx @@ -5,30 +5,21 @@ import { Link, useLocation } from 'react-router-dom-v5-compat'; import { GrafanaTheme2, IconName, locationUtil } from '@grafana/data'; import { Icon, useStyles2 } from '@grafana/ui'; +import { isCurrentPath } from './scopeNavgiationUtils'; + export interface ScopesNavigationTreeLinkProps { to: string; title: string; id: string; } -// Helper function to get the base path for a dashboard URL for comparison purposes. -// e.g., /d/dashboardId/slug -> /d/dashboardId -// /d/dashboardId -> /d/dashboardId -function getDashboardPathForComparison(pathname: string): string { - return pathname.split('/').slice(0, 3).join('/'); -} - export function ScopesNavigationTreeLink({ to, title, id }: ScopesNavigationTreeLinkProps) { const styles = useStyles2(getStyles); const linkIcon = useMemo(() => getLinkIcon(to), [to]); - const isDashboard = to.startsWith('/d/'); const locPathname = useLocation().pathname; - // For dashboards, the title is appended to the path when we navigate to just the dashboard id, hence we need to disregard this - const currentPath = isDashboard ? getDashboardPathForComparison(locPathname) : locPathname; - // Ignore query params - const isCurrent = to.split('?')[0] === currentPath; + const isCurrent = isCurrentPath(locPathname, to); return ( { + it('should return the correct path for a dashboard', () => { + expect(getDashboardPathForComparison('/d/dashboardId/slug')).toBe('/d/dashboardId'); + expect(getDashboardPathForComparison('/d/dashboardId')).toBe('/d/dashboardId'); + expect(getDashboardPathForComparison('/d/dashboardId/slug?query=param')).toBe('/d/dashboardId'); + }); + + it('should return the correct path for a navigation', () => { + expect(isCurrentPath('/d/dashboardId/slug', '/d/dashboardId')).toBe(true); + expect(isCurrentPath('/d/dashboardId', '/d/dashboardId')).toBe(true); + }); + + it('shoudl handle non-dashboard paths', () => { + expect(isCurrentPath('/other/path', '/other/path')).toBe(true); + expect(isCurrentPath('/other/path', '/other/path?query=param')).toBe(true); + expect(isCurrentPath('/other/path', '/other/path#hash')).toBe(true); + expect(isCurrentPath('/other/path', '/other/path?query=param#hash')).toBe(true); + }); + + it('should return the correct path for a navigation with query params', () => { + expect(isCurrentPath('/d/dashboardId/slug', '/d/dashboardId?query=param')).toBe(true); + expect(isCurrentPath('/d/dashboardId', '/d/dashboardId?query=param')).toBe(true); + }); + + it('should return the correct path for a navigation with hash', () => { + expect(isCurrentPath('/d/dashboardId/slug', '/d/dashboardId#hash')).toBe(true); + expect(isCurrentPath('/d/dashboardId', '/d/dashboardId#hash')).toBe(true); + }); +}); diff --git a/public/app/features/scopes/dashboards/scopeNavgiationUtils.ts b/public/app/features/scopes/dashboards/scopeNavgiationUtils.ts new file mode 100644 index 00000000000..fe6d458a312 --- /dev/null +++ b/public/app/features/scopes/dashboards/scopeNavgiationUtils.ts @@ -0,0 +1,24 @@ +// Helper function to get the base path for a dashboard URL for comparison purposes. +// e.g., /d/dashboardId/slug -> /d/dashboardId +// /d/dashboardId -> /d/dashboardId +export function getDashboardPathForComparison(pathname: string): string { + return pathname.split('/').slice(0, 3).join('/'); +} + +export function normalizePath(path: string): string { + // Remove query + hash + trailing slash (except root) + const noQuery = path.split('?')[0].split('#')[0]; + return noQuery !== '/' && noQuery.endsWith('/') ? noQuery.slice(0, -1) : noQuery; +} + +// Pathname comes from location.pathname +export function isCurrentPath(pathname: string, to: string): boolean { + const isDashboard = to.startsWith('/d/'); + + if (isDashboard) { + // For dashboards, the title is appended to the path when we navigate to just the dashboard id, hence we need to disregard this + return getDashboardPathForComparison(pathname) === normalizePath(to); + } + //Ignore query params + return pathname === normalizePath(to); +} diff --git a/public/app/features/scopes/selector/ScopesSelectorService.test.ts b/public/app/features/scopes/selector/ScopesSelectorService.test.ts index 5721d1daaf8..7b636ffa696 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.test.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.test.ts @@ -1,11 +1,22 @@ import { Scope, ScopeNode, Store } from '@grafana/data'; +import { locationService } from '@grafana/runtime'; import { ScopesApiClient } from '../ScopesApiClient'; import { ScopesDashboardsService } from '../dashboards/ScopesDashboardsService'; +import { ScopeNavigation } from '../dashboards/types'; import { RECENT_SCOPES_KEY, ScopesSelectorService } from './ScopesSelectorService'; import { RecentScope } from './types'; +// Mock locationService +jest.mock('@grafana/runtime', () => ({ + ...jest.requireActual('@grafana/runtime'), + locationService: { + push: jest.fn(), + getLocation: jest.fn(), + }, +})); + describe('ScopesSelectorService', () => { let service: ScopesSelectorService; let apiClient: jest.Mocked; @@ -40,6 +51,12 @@ describe('ScopesSelectorService', () => { let store: Store; beforeEach(() => { + // Clear all mocks + jest.clearAllMocks(); + + // Mock locationService to return a default location + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-page' }); + apiClient = { fetchScope: jest.fn().mockResolvedValue(mockScope), fetchMultipleScopes: jest.fn().mockResolvedValue([mockScope]), @@ -55,7 +72,17 @@ describe('ScopesSelectorService', () => { } as unknown as jest.Mocked; dashboardsService = { - fetchDashboards: jest.fn(), + fetchDashboards: jest.fn().mockResolvedValue(undefined), + state: { + scopeNavigations: [], + dashboards: [], + drawerOpened: false, + filteredFolders: {}, + folders: {}, + forScopeNames: [], + loading: false, + searchQuery: '', + }, } as unknown as jest.Mocked; storeValue = {}; @@ -532,4 +559,155 @@ describe('ScopesSelectorService', () => { expect(recentScopes[2][0].parentNode).toBeUndefined(); // invalid parent node should be removed }); }); + + describe('redirect on scope selection', () => { + it('should redirect to the first scopeNavigation with /d/ URL when current URL is not a scopeNavigation', async () => { + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'test-scope', + url: '/d/dashboard1', + }, + status: { + title: 'Dashboard 1', + groups: [], + }, + metadata: { + name: 'dashboard1', + }, + }, + ]; + + dashboardsService.state.scopeNavigations = mockNavigations; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); + + await service.changeScopes(['test-scope']); + + expect(locationService.push).toHaveBeenCalledWith('/d/dashboard1'); + }); + + it('should NOT redirect when the first scopeNavigation does not contain /d/ (e.g., logs drilldown)', async () => { + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'test-scope', + url: '/explore', + }, + status: { + title: 'Explore', + groups: [], + }, + metadata: { + name: 'explore1', + }, + }, + ]; + + dashboardsService.state.scopeNavigations = mockNavigations; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); + + await service.changeScopes(['test-scope']); + + expect(locationService.push).not.toHaveBeenCalled(); + }); + + it('should NOT redirect when current URL matches a scopeNavigation', async () => { + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'test-scope', + url: '/d/dashboard1', + }, + status: { + title: 'Dashboard 1', + groups: [], + }, + metadata: { + name: 'dashboard1', + }, + }, + ]; + + dashboardsService.state.scopeNavigations = mockNavigations; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/d/dashboard1' }); + + await service.changeScopes(['test-scope']); + + expect(locationService.push).not.toHaveBeenCalled(); + }); + + it('should NOT redirect when there are no scopeNavigations', async () => { + dashboardsService.state.scopeNavigations = []; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); + + await service.changeScopes(['test-scope']); + + expect(locationService.push).not.toHaveBeenCalled(); + }); + + it('should NOT redirect when scopeNavigation does not have a url property', async () => { + const mockNavigations = [ + { + spec: { + scope: 'test-scope', + // Missing url property + }, + status: { + title: 'Dashboard 1', + groups: [], + }, + metadata: { + name: 'dashboard1', + }, + }, + ] as unknown as ScopeNavigation[]; + + dashboardsService.state.scopeNavigations = mockNavigations; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); + + await service.changeScopes(['test-scope']); + + expect(locationService.push).not.toHaveBeenCalled(); + }); + + it('should handle multiple scopeNavigations and redirect to the first dashboard one', async () => { + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'test-scope', + url: '/d/first-dashboard', + }, + status: { + title: 'First Dashboard', + groups: [], + }, + metadata: { + name: 'first-dashboard', + }, + }, + { + spec: { + scope: 'test-scope', + url: '/d/second-dashboard', + }, + status: { + title: 'Second Dashboard', + groups: [], + }, + metadata: { + name: 'second-dashboard', + }, + }, + ]; + + dashboardsService.state.scopeNavigations = mockNavigations; + (locationService.getLocation as jest.Mock).mockReturnValue({ pathname: '/some-other-page' }); + + await service.changeScopes(['test-scope']); + + // Should redirect to the first one + expect(locationService.push).toHaveBeenCalledWith('/d/first-dashboard'); + expect(locationService.push).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/public/app/features/scopes/selector/ScopesSelectorService.ts b/public/app/features/scopes/selector/ScopesSelectorService.ts index 7e8f1ae060a..e0705a31f2d 100644 --- a/public/app/features/scopes/selector/ScopesSelectorService.ts +++ b/public/app/features/scopes/selector/ScopesSelectorService.ts @@ -1,11 +1,12 @@ import { Scope, ScopeNode, store as storeImpl } from '@grafana/data'; -import { config } from '@grafana/runtime'; +import { config, locationService } from '@grafana/runtime'; import { SceneRenderProfiler } from '@grafana/scenes'; import { getDashboardSceneProfiler } from 'app/features/dashboard/services/DashboardProfiler'; import { ScopesApiClient } from '../ScopesApiClient'; import { ScopesServiceBase } from '../ScopesServiceBase'; import { ScopesDashboardsService } from '../dashboards/ScopesDashboardsService'; +import { isCurrentPath } from '../dashboards/scopeNavgiationUtils'; import { closeNodes, @@ -18,7 +19,6 @@ import { treeNodeAtPath, } from './scopesTreeUtils'; import { NodesMap, RecentScope, RecentScopeSchema, ScopeSchema, ScopesMap, SelectedScope, TreeNode } from './types'; - export const RECENT_SCOPES_KEY = 'grafana.scopes.recent'; export interface ScopesSelectorServiceState { @@ -362,7 +362,31 @@ export class ScopesSelectorService extends ScopesServiceBase s.scopeId)); + this.dashboardsService.fetchDashboards(scopes.map((s) => s.scopeId)).then(() => { + // Redirect to first scopeNavigation if current URL isn't a scopeNavigation + const currentPath = locationService.getLocation().pathname; + const activeScopeNavigation = this.dashboardsService.state.scopeNavigations.find((s) => { + if (!('url' in s.spec) || typeof s.spec.url !== 'string') { + return false; + } + return isCurrentPath(currentPath, s.spec.url); + }); + + if (!activeScopeNavigation && this.dashboardsService.state.scopeNavigations.length > 0) { + // Redirect to the first available scopeNavigation + const firstScopeNavigation = this.dashboardsService.state.scopeNavigations[0]; + + if ( + firstScopeNavigation && + 'url' in firstScopeNavigation.spec && + typeof firstScopeNavigation.spec.url === 'string' && + // Only redirect to dashboards TODO: Remove this once Logs Drilldown has Scopes support + firstScopeNavigation.spec.url.includes('/d/') + ) { + locationService.push(firstScopeNavigation.spec.url); + } + } + }); if (scopes.length > 0) { const fetchedScopes = await this.apiClient.fetchMultipleScopes(scopes.map((s) => s.scopeId)); diff --git a/public/app/features/scopes/tests/dashboardsList.test.ts b/public/app/features/scopes/tests/dashboardsList.test.ts index 0e53733658a..34b754b37dd 100644 --- a/public/app/features/scopes/tests/dashboardsList.test.ts +++ b/public/app/features/scopes/tests/dashboardsList.test.ts @@ -4,6 +4,7 @@ import { config, locationService } from '@grafana/runtime'; import { ScopesService } from '../ScopesService'; import { ScopesDashboardsService } from '../dashboards/ScopesDashboardsService'; +import { ScopeNavigation } from '../dashboards/types'; import { clearNotFound, @@ -261,6 +262,35 @@ describe('Dashboards list', () => { expectDashboardLength('billing-usage', 1); }); + it('redirects to the first scope navigation if your current dashboard is not a scope navigation', async () => { + // Render another dashboard, which is not a scope navigation + const mockNavigations: ScopeNavigation[] = [ + { + spec: { + scope: 'grafana', + url: '/d/dashboard1', + }, + status: { + title: 'Dashboard 1', + groups: ['group1'], + }, + metadata: { + name: 'dashboard1', + }, + }, + ]; + fetchDashboardsSpy.mockResolvedValue(mockNavigations); + + await renderDashboard(); + expect(locationService.getLocation().pathname).toBe('/'); + + await updateScopes(scopesService, ['grafana']); + expect(locationService.getLocation().pathname).toBe('/d/dashboard1'); + // renderDashboard defaults to home dashboard + expect(locationService.getLocation().pathname).not.toBe('/'); + expect(fetchDashboardsSpy).toHaveBeenCalled(); + }); + it('Shows a proper message when no scopes are selected', async () => { await toggleDashboards(); expectNoDashboardsNoScopes();