OCPBUGS-84047: Fix perspective stuck issue with transition guard#16411
OCPBUGS-84047: Fix perspective stuck issue with transition guard#16411TheRealJon wants to merge 1 commit intoopenshift:mainfrom
Conversation
Fixes perspective getting stuck when switching away from plugin-registered perspectives (e.g., Fleet Management). Plugins watch activePerspective changes and force back to their perspective, preventing user-initiated switches. Root cause: Plugin effects fire during perspective transitions and override the user's switch by calling setActivePerspective with the plugin's perspective. Solution: Add transition guard (isTransitioning ref) that blocks setPerspective calls during active transitions, preventing plugin interference. Additional fixes: - Extract getPathWithoutPerspectiveParam to utils.ts to eliminate duplication - Strip only ?perspective= param to prevent loops while preserving other query params and hash - Navigate before updating perspective state to prevent plugins from seeing mismatch - Remove location from effect deps to prevent firing on every navigation - Restore ACM default on initial load (when no previous perspective exists) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-84047, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-84047, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Console Approver Docs Approver: PX Approver: |
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-84047, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis PR modifies the perspective detection flow to strip the 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/detect-perspective/__tests__/PerspectiveDetector.spec.tsx (1)
55-57: ⚡ Quick winAssert the exact number of perspective updates.
toHaveBeenCalledWith(...)still passes if the detector emits an extra fallback or duplicate update. Since this bug is about competing effects, these tests should also lock down the call count.Suggested tightening
await waitFor(() => { + expect(setActivePerspective).toHaveBeenCalledTimes(1); expect(setActivePerspective).toHaveBeenCalledWith('admin', '/'); });Apply the same assertion to the other updated cases as well.
Also applies to: 73-75, 91-93, 139-141, 162-165
🤖 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/__tests__/PerspectiveDetector.spec.tsx` around lines 55 - 57, The test in PerspectiveDetector.spec.tsx currently only asserts setActivePerspective was called with specific args, which allows extra/duplicate calls to slip by; update each expectation (the blocks around the existing expects at the mentioned locations and all other cases where setActivePerspective is asserted) to also assert the exact call count, e.g., use setActivePerspective.toHaveBeenCalledTimes(1) in addition to toHaveBeenCalledWith('admin', '/') so the detector cannot emit extra fallback/duplicate updates; apply this tightening to the other assertion sites referenced (the assertions around lines 73-75, 91-93, 139-141, 162-165) to lock down call counts for all scenarios.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@frontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts`:
- Around line 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.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/detect-perspective/__tests__/PerspectiveDetector.spec.tsx`:
- Around line 55-57: The test in PerspectiveDetector.spec.tsx currently only
asserts setActivePerspective was called with specific args, which allows
extra/duplicate calls to slip by; update each expectation (the blocks around the
existing expects at the mentioned locations and all other cases where
setActivePerspective is asserted) to also assert the exact call count, e.g., use
setActivePerspective.toHaveBeenCalledTimes(1) in addition to
toHaveBeenCalledWith('admin', '/') so the detector cannot emit extra
fallback/duplicate updates; apply this tightening to the other assertion sites
referenced (the assertions around lines 73-75, 91-93, 139-141, 162-165) to lock
down call counts for all scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4a899db9-d78b-4614-ab1f-78dfe96d672e
📒 Files selected for processing (5)
frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsxfrontend/packages/console-app/src/components/detect-perspective/PerspectiveDetector.tsxfrontend/packages/console-app/src/components/detect-perspective/__tests__/PerspectiveDetector.spec.tsxfrontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.tsfrontend/packages/console-app/src/components/detect-perspective/utils.ts
📜 Review details
🔇 Additional comments (1)
frontend/packages/console-app/src/components/detect-perspective/utils.ts (1)
14-18: Nice extraction of the URL sanitizing logic.This keeps the
perspectivestripping behavior consistent across both call sites while still preserving the rest of the URL state.
| // 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); |
There was a problem hiding this comment.
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.
|
/retest-required |
|
@TheRealJon: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Fixes perspective getting stuck when switching away from plugin-registered perspectives (e.g., Fleet Management). Plugins watch activePerspective changes and force back to their perspective, preventing user-initiated switches.
Analysis / Root cause
Plugin effects fire during perspective transitions and override the user's switch by calling setActivePerspective with the plugin's perspective.
Solution description
Add transition guard (isTransitioning ref) that blocks setPerspective calls during active transitions, preventing plugin interference.
Screenshots / screen recording:
Screen.Recording.2026-05-07.at.2.59.38.PM.mov
Test setup
Test cases
Test Case 1: Switching Away from Plugin Perspective (Primary Bug Fix)
Objective: Verify user can switch away from Fleet Management perspective
Steps:
Expected Result:
Failure Scenario (without fix):
Test Case 2: Switching TO Plugin Perspective
Objective: Verify switching TO Fleet Management still works
Steps:
Expected Result:
--- ## Test Case 3: Rapid Perspective Switching
Objective: Verify transition guard handles rapid clicks
Steps:
Expected Result:
Test Case 4: Query Param Preservation
Objective: Verify query params (except ?perspective=) are preserved during switches
Steps:
Expected Result:
Note: The navigation goes to perspective landing page (/ or /dashboards), so query params from previous page aren't expected to transfer. The fix ensures we don't carry over ?perspective= which caused loops.
Test Case 5: Hash Fragment Preservation
Objective: Verify URL hash is handled correctly
Steps:
Expected Result:
Test Case 6: Initial Load with ACM Installed
Objective: Verify ACM default on first visit
Steps:
Expected Result:
Test Case 7: Built-in Perspective Switching (Regression Test)
Objective: Verify switching between Admin/Dev still works normally
Steps:
Expected Result:
Test Case 8: Browser Console Check
Objective: Verify no unexpected errors or warnings
Steps:
Expected Result:
Test Case 9: Multiple Plugin Perspectives
Objective: If multiple plugins register perspectives, verify switching works
Steps:
Expected Result:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit