Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import type { FC } from 'react';
import { useEffect } from 'react';
import { createPath, useLocation } from 'react-router';
import { useLocation } from 'react-router';
import type { Perspective } from '@console/dynamic-plugin-sdk';
import { PerspectiveContext } from '@console/dynamic-plugin-sdk';
import { LoadingBox } from '@console/shared/src/components/loading/LoadingBox';
import { usePerspectives } from '@console/shared/src/hooks/usePerspectives';
import PerspectiveDetector from './PerspectiveDetector';
import { useValuesForPerspectiveContext } from './useValuesForPerspectiveContext';
import { getPathWithoutPerspectiveParam } from './utils';

type DetectPerspectiveProps = {
children: React.ReactNode;
Expand All @@ -29,9 +30,13 @@ const DetectPerspective: FC<DetectPerspectiveProps> = ({ children }) => {
const location = useLocation();
useEffect(() => {
if (perspectiveParam && perspectiveParam !== activePerspective) {
setActivePerspective(perspectiveParam, createPath(location));
// Strip ?perspective= param to avoid loop, but preserve other query params and hash
setActivePerspective(perspectiveParam, getPathWithoutPerspectiveParam(location));
}
}, [perspectiveParam, activePerspective, setActivePerspective, location]);
// location is intentionally excluded from deps to prevent firing on every navigation
// The effect should only run when perspectiveParam changes (URL param added/changed)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [perspectiveParam, activePerspective, setActivePerspective]);

return loaded ? (
activePerspective ? (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import type { FC } from 'react';
import { useEffect, useState } from 'react';
import { useLocation, createPath } from 'react-router';
import { useLocation } from 'react-router';
import type { Perspective, ResolvedExtension } from '@console/dynamic-plugin-sdk';
import { usePerspectives } from '@console/shared/src/hooks/usePerspectives';
import { getPathWithoutPerspectiveParam } from './utils';

type DetectorProps = {
setActivePerspective: (perspective: string, next: string) => void;
Expand Down Expand Up @@ -40,18 +41,22 @@ const Detector: FC<DetectorProps> = ({
});

useEffect(() => {
const pathWithoutPerspectiveParam = getPathWithoutPerspectiveParam(location);
if (detectedPerspective) {
setActivePerspective(detectedPerspective, createPath(location));
// Strip ?perspective= param to avoid loop, but preserve other query params and hash
setActivePerspective(detectedPerspective, pathWithoutPerspectiveParam);
} else if (defaultPerspective && (detectors.length < 1 || detectionComplete)) {
// set default perspective if there are no detectors or none of the detections were successful
setActivePerspective(defaultPerspective.properties.id, createPath(location));
setActivePerspective(defaultPerspective.properties.id, pathWithoutPerspectiveParam);
}
// location is intentionally excluded from deps to prevent firing on every navigation
// The effect should only run when detection completes or changes
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
defaultPerspective,
detectedPerspective,
detectionComplete,
detectors.length,
location,
setActivePerspective,
]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('PerspectiveDetector', () => {
render(<PerspectiveDetector setActivePerspective={setActivePerspective} />);

await waitFor(() => {
expect(setActivePerspective).toHaveBeenCalledWith('admin', '');
expect(setActivePerspective).toHaveBeenCalledWith('admin', '/');
});
});

Expand All @@ -71,7 +71,7 @@ describe('PerspectiveDetector', () => {
promiseResolver(() => [true, false]);

await waitFor(() => {
expect(setActivePerspective).toHaveBeenCalledWith('dev', '');
expect(setActivePerspective).toHaveBeenCalledWith('dev', '/');
});
});

Expand All @@ -89,7 +89,7 @@ describe('PerspectiveDetector', () => {
promiseResolver(() => [false, false]);

await waitFor(() => {
expect(setActivePerspective).toHaveBeenCalledWith('admin', '');
expect(setActivePerspective).toHaveBeenCalledWith('admin', '/');
});
});

Expand Down Expand Up @@ -137,11 +137,11 @@ describe('PerspectiveDetector', () => {
promiseResolver(() => [false, false]);

await waitFor(() => {
expect(setActivePerspective).toHaveBeenCalledWith('admin', '');
expect(setActivePerspective).toHaveBeenCalledWith('admin', '/');
});
});

it('preserves query and hash when setting perspective', async () => {
it('preserves query params and hash but strips perspective param when setting perspective', async () => {
let promiseResolver: (value: () => [boolean, boolean]) => void;
const testPromise = new Promise<() => [boolean, boolean]>(
(resolver) => (promiseResolver = resolver),
Expand All @@ -151,7 +151,7 @@ describe('PerspectiveDetector', () => {
(usePerspectives as jest.Mock).mockImplementation(() => mockPerspectives);
useLocationMock.mockImplementation(() => ({
pathname: '/some/path',
search: '?query=param',
search: '?query=param&perspective=old',
hash: '#some-hash',
}));

Expand All @@ -160,6 +160,7 @@ describe('PerspectiveDetector', () => {
promiseResolver(() => [true, false]);

await waitFor(() => {
// Preserves query params and hash, but strips ?perspective= to avoid loop
expect(setActivePerspective).toHaveBeenCalledWith('dev', '/some/path?query=param#some-hash');
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useCallback, useState } from 'react';
import { useCallback, useState, useRef } from 'react';
import { useNavigate } from 'react-router';
import type { PerspectiveType, UseActivePerspective } from '@console/dynamic-plugin-sdk';
import {
Expand Down Expand Up @@ -27,20 +27,43 @@ export const useValuesForPerspectiveContext = (): [
const latestPerspective = loaded && (preferredPerspective || lastPerspective);
const acmPerspectiveExtension = usePerspectiveExtension(ACM_PERSPECTIVE_ID);
const existingPerspective = activePerspective || latestPerspective;
// Default to ACM perspective on initial load if no previous perspective exists
// The transition guard prevents this from interfering during user-initiated switches
const perspective =
!!acmPerspectiveExtension && !existingPerspective
? ACM_PERSPECTIVE_ID
: existingPerspective || '';
const isValidPerspective =
loaded && perspectiveExtensions.some((p) => p.properties.id === perspective);

// Guard to prevent plugins from forcing perspective during transitions
const isTransitioning = useRef(false);

const setPerspective = useCallback<SetActivePerspective>(
(newPerspective, next) => {
// Ignore calls during transitions - plugins trying to force perspective back
if (isTransitioning.current) {
return;
}

// Set guard to block plugin interference
isTransitioning.current = true;

// Navigate FIRST, then update perspective state
// This prevents plugins from seeing activePerspective change while still on old route
// which triggers their perspective-forcing logic
navigate(next || '/');

// Update perspective state after navigation starts
setLastPerspective(newPerspective);
setActivePerspective(newPerspective);
// Navigate to next or root and let the default page determine where to go to next
navigate(next || '/');
fireTelemetryEvent('Perspective Changed', { perspective: newPerspective });

// Clear guard after navigation and state updates complete
// Use setTimeout to ensure this runs after all synchronous effects
setTimeout(() => {
isTransitioning.current = false;
}, 0);
Comment on lines +49 to +66
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make the transition guard exception-safe.

Once isTransitioning.current flips to true, any synchronous throw from navigate, setLastPerspective, or fireTelemetryEvent leaves perspective switching disabled until reload. Wrap the guarded block in try/finally so the ref is always released.

Suggested fix
       // Set guard to block plugin interference
       isTransitioning.current = true;

-      // Navigate FIRST, then update perspective state
-      // This prevents plugins from seeing activePerspective change while still on old route
-      // which triggers their perspective-forcing logic
-      navigate(next || '/');
-
-      // Update perspective state after navigation starts
-      setLastPerspective(newPerspective);
-      setActivePerspective(newPerspective);
-      fireTelemetryEvent('Perspective Changed', { perspective: newPerspective });
-
-      // Clear guard after navigation and state updates complete
-      // Use setTimeout to ensure this runs after all synchronous effects
-      setTimeout(() => {
-        isTransitioning.current = false;
-      }, 0);
+      try {
+        // Navigate FIRST, then update perspective state
+        // This prevents plugins from seeing activePerspective change while still on old route
+        // which triggers their perspective-forcing logic
+        navigate(next || '/');
+
+        // Update perspective state after navigation starts
+        setLastPerspective(newPerspective);
+        setActivePerspective(newPerspective);
+        fireTelemetryEvent('Perspective Changed', { perspective: newPerspective });
+      } finally {
+        // Clear guard after navigation and state updates complete
+        // Use setTimeout to ensure this runs after all synchronous effects
+        setTimeout(() => {
+          isTransitioning.current = false;
+        }, 0);
+      }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@frontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts`
around lines 49 - 66, The transition guard currently sets
isTransitioning.current = true then calls navigate, setLastPerspective,
setActivePerspective and fireTelemetryEvent, but if any of those throw
synchronously the guard stays true; wrap the sequence from navigate through
fireTelemetryEvent in a try/finally so that isTransitioning.current is always
set back to false in the finally block (keeping the initial set to true before
the try), ensuring the ref is released even on exceptions.

},
[setLastPerspective, setActivePerspective, navigate, fireTelemetryEvent],
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type { Location } from 'react-router';
import { createPath } from 'react-router';

/**
* Strip ?perspective= param to prevent loops, but preserve other query params and hash.
*
* The ?perspective= param can create a redirect loop when passed through navigation.
* This helper ensures we preserve all other query params and the hash fragment while
* removing only the problematic perspective param.
*
* @param location - The current location object from useLocation()
* @returns Path string with perspective param removed (e.g., "/path?other=value#hash")
*/
export const getPathWithoutPerspectiveParam = (location: Location): string => {
const path = createPath(location);
const url = new URL(path, window.location.origin);
url.searchParams.delete('perspective');
return url.pathname + url.search + url.hash;
};