OCPBUGS-85121: Skip redundant navigate() in setPerspective when target matches current URL#16683
OCPBUGS-85121: Skip redundant navigate() in setPerspective when target matches current URL#16683TheRealJon wants to merge 2 commits into
Conversation
…t matches current URL setActivePerspective() always called navigate() unconditionally, causing redundant page reloads for custom perspective plugins. When the target URL matched the current location, this triggered namespace handler validation loops and visible re-renders. This also stripped the ?perspective= param in DetectContext to prevent it from persisting and re-triggering the effect. Adapted from PR openshift#16410 by logonoff after files were merged into DetectContext. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
jira/valid-reference
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-85121, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
WalkthroughPerspective handling now removes the ChangesPerspective URL flow
Sequence Diagram(s)Perspective parameter cleanup sequenceDiagram
participant DetectContext
participant react-router
participant window.location
DetectContext->>window.location: read perspective query param
DetectContext->>react-router: createPath(cleaned location)
alt perspective differs from activePerspective
DetectContext->>DetectContext: update activePerspective
else perspective matches activePerspective
DetectContext->>react-router: navigate(cleanedPath, { replace: true })
end
Perspective setter guard sequenceDiagram
participant useValuesForPerspectiveContext
participant react-router
participant window.location
useValuesForPerspectiveContext->>window.location: read current location
useValuesForPerspectiveContext->>react-router: createPath(window.location)
alt targetPath differs from current path
useValuesForPerspectiveContext->>react-router: navigate(targetPath)
else targetPath matches current path
useValuesForPerspectiveContext-->>useValuesForPerspectiveContext: skip navigate()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@TheRealJon: This pull request references Jira Issue OCPBUGS-85121, 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. |
|
[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
![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
🤖 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-context/useValuesForPerspectiveContext.ts`:
- Around line 41-44: The navigation guard in useValuesForPerspectiveContext is
comparing next against createPath(window.location), but next is built from the
router location and is basename-relative, so this can incorrectly trigger
navigate on the same route. Update the comparison to use the current router
location from useLocation() instead of window.location, and make sure the
location value is included in the callback dependencies alongside navigate and
createPath.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23758d72-fae5-4e63-8584-9ee6616614ff
📒 Files selected for processing (3)
frontend/packages/console-app/src/components/detect-context/DetectContext.tsxfrontend/packages/console-app/src/components/detect-context/__tests__/DetectContext.spec.tsxfrontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts
| // Only navigate if a path is provided to avoid unnecessary navigation | ||
| if (next && next !== createPath(window.location)) { | ||
| navigate(next); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How basename is configured for the router and how setActivePerspective `next` args are produced
rg -nP '\bbasename\b' frontend/public frontend/packages -g '*.ts*' -C2
rg -nP 'setActivePerspective\s*\(' frontend/public frontend/packages -g '*.ts*' -C2
rg -nP 'createPath\s*\(' frontend/packages -g '*.ts*' -C2Repository: openshift/console
Length of output: 9757
🏁 Script executed:
sed -n '1,220p' frontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts
printf '\n---\n'
sed -n '1,240p' frontend/packages/console-app/src/components/detect-context/DetectContext.tsx
printf '\n---\n'
sed -n '1,140p' frontend/packages/console-app/src/components/detect-context/PerspectiveDetector.tsx
printf '\n---\n'
sed -n '1,120p' frontend/public/components/app.tsxRepository: openshift/console
Length of output: 19626
🏁 Script executed:
sed -n '1,220p' frontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts
printf '\n---\n'
sed -n '1,240p' frontend/packages/console-app/src/components/detect-context/DetectContext.tsx
printf '\n---\n'
sed -n '1,140p' frontend/packages/console-app/src/components/detect-context/PerspectiveDetector.tsx
printf '\n---\n'
sed -n '330,360p' frontend/public/components/app.tsxRepository: openshift/console
Length of output: 14933
🌐 Web query:
React Router BrowserRouter basename useLocation pathname basename-relative createPath window.location
💡 Result:
In React Router (v6 and later), the basename is integrated into the routing system to simplify path management [1][2]. When you configure a BrowserRouter with a basename (e.g., ), the router handles the basename automatically for most routing operations [2][3]. Key behaviors regarding basename and paths: 1. useLocation and basename: The useLocation hook returns a location object where the basename is stripped from the pathname [1]. For instance, if the browser URL is /app/home, useLocation will return a pathname of /home [1]. This ensures that your routes, defined as path="/home", match correctly without manual adjustment [1]. 2. Raw path access: If you specifically need the "full" URL including the basename, you should rely on the browser's native window.location.pathname [4]. The location object provided by React Router is intentionally abstracted to be relative to the application's root (the basename). 3. Relative Path Resolution (useResolvedPath and createPath): - The useResolvedPath hook resolves relative paths against the current location's pathname [5][6]. Because React Router handles the basename internally, paths resolved via this hook correctly respect the application structure [3]. - The createPath utility creates a string URL path from pathname, search, and hash components [7]. It is generally used for constructing paths to be consumed by navigation APIs; it does not automatically prepend a basename unless that basename is part of the provided path structure or handled by the router's internal navigation logic [8]. 4. Best Practices: - Avoid using window.location.pathname for routing logic inside your components; always use hooks like useLocation and useResolvedPath to ensure compatibility with your basename configuration [4]. - When performing manual path matching, use the location object provided by the router, as it provides the basename-stripped path, ensuring your logic remains consistent across different environments (e.g., development vs. production under different sub-paths) [4]. In summary, React Router v6+ abstracts the basename away from the location pathname, making it easier to write path-agnostic code while maintaining correct routing behavior [1][4].
Citations:
- 1: Is it possible to get pathname relative to basename ? remix-run/react-router#8068
- 2: https://reactrouter.com/6.19.0/router-components/browser-router
- 3: https://remix-run-react-router.mintlify.app/api/hooks/use-resolved-path
- 4: https://stackoverflow.com/questions/56302254/basename-does-not-seem-to-be-taken-into-account-with-matchpath-in-react-router
- 5: https://reactrouter.com/6.30.3/hooks/use-resolved-path
- 6: https://reactrouter.com/api/hooks/useResolvedPath
- 7: https://reactrouter.com/api/utils/createPath
- 8: https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/hooks.tsx
🏁 Script executed:
rg -n "createPath\(window\.location\)|window\.location\.pathname|createPath\(location\)" frontend/packages/console-app frontend/public frontend/packages/console-shared -C2Repository: openshift/console
Length of output: 8225
🌐 Web query:
React Router basename useLocation pathname includes basename or stripped createPath(location)
💡 Result:
In React Router (v6 and later), the behavior of useLocation and basename is as follows: 1. useLocation pathname: The pathname returned by the useLocation hook has the basename stripped [1]. React Router automatically removes the configured basename from the current URL pathname so that your application code can work with paths relative to your application root [1][2]. 2. createPath behavior: The createPath utility function is a low-level helper that simply takes a partial location object (containing pathname, search, and hash) and joins them into a string [3]. It does not automatically prepend or strip a basename; it is basename-agnostic [3]. When navigating or creating links, React Router’s internal navigation logic (such as the navigate function or Link components) handles the basename automatically by prepending it when performing actions [4][5]. If you are manually constructing paths for navigation and want to respect the basename, you should rely on React Router's navigation hooks or utilities that are aware of the router context rather than manually appending the basename yourself [4][5]. If you specifically need to use createPath in a scenario involving a basename, you must manually prepend the basename to the pathname if necessary, as createPath will not perform this transformation for you [4][5].
Citations:
- 1: Is it possible to get pathname relative to basename ? remix-run/react-router#8068
- 2: https://stackoverflow.com/questions/56302254/basename-does-not-seem-to-be-taken-into-account-with-matchpath-in-react-router
- 3: https://reactrouter.com/api/utils/createPath
- 4: https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/hooks.tsx
- 5: https://github.com/remix-run/react-router/blob/a3e4b8ed875611637357647fcf862c2bc61f4e11/packages/react-router/lib/dom/lib.tsx
🏁 Script executed:
rg -n '"react-router|react-router-dom"' frontend/package.json frontend/packages/**/package.json -C1Repository: openshift/console
Length of output: 321
Compare against the router location, not window.location. This app uses BrowserRouter basename={window.SERVER_FLAGS.basePath}, and the next values passed here are built from createPath(location), so they’re basename-relative. window.location includes the basename, which makes the guard miss same-route matches and trigger an unnecessary navigate. Use useLocation() for the comparison and add it to the callback deps.
🤖 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-context/useValuesForPerspectiveContext.ts`
around lines 41 - 44, The navigation guard in useValuesForPerspectiveContext is
comparing next against createPath(window.location), but next is built from the
router location and is basename-relative, so this can incorrectly trigger
navigate on the same route. Update the comparison to use the current router
location from useLocation() instead of window.location, and make sure the
location value is included in the callback dependencies alongside navigate and
createPath.
|
/hold Needs more verification. I think this breaks the behavior where we automatically navigate to the landing page for the perspective you switch to. |
openshift-ci
Bot
added
the
do-not-merge/hold
Previous fix prevented redundant navigations but broke perspective switching. When user switches perspective via UI, setPerspective gets called with no next path. Code skipped navigate entirely, preventing landing page redirect. Now navigate to '/' when perspective changes and no explicit path provided. Router handles landing page redirect. Still skip navigate when perspective unchanged to avoid redundant reloads. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts (1)
39-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the router location for the no-op navigation guard.
Line 45 compares a router-relative
targetPathagainstcreatePath(window.location). Under aBrowserRouter basename, that can miss same-route matches and still callnavigate, which brings back the redundant reload/re-render path this fix is trying to remove. Compare againstcreatePath(location)fromuseLocation()and addlocationto the callback deps.Suggested fix
+ const location = useLocation(); const setPerspective = useCallback<SetActivePerspective>( (newPerspective, next) => { const perspectiveChanged = newPerspective !== perspective; setLastPerspective(newPerspective); setActivePerspective(newPerspective); // Only navigate if perspective changed if (perspectiveChanged) { const targetPath = next || '/'; - if (targetPath !== createPath(window.location)) { + if (targetPath !== createPath(location)) { navigate(targetPath); } } fireTelemetryEvent('Perspective Changed', { perspective: newPerspective }); }, - [setLastPerspective, setActivePerspective, navigate, fireTelemetryEvent, perspective], + [setLastPerspective, setActivePerspective, navigate, fireTelemetryEvent, perspective, location], );🤖 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-context/useValuesForPerspectiveContext.ts` around lines 39 - 46, The no-op navigation guard in useValuesForPerspectiveContext should compare the computed targetPath against the router’s current location instead of window.location, since BrowserRouter basename can make same-route checks miss and still trigger navigate. Update the guard to use location from useLocation() inside the callback, and include location in the callback’s dependency list so the check stays in sync with router state.
🤖 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.
Duplicate comments:
In
`@frontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts`:
- Around line 39-46: The no-op navigation guard in
useValuesForPerspectiveContext should compare the computed targetPath against
the router’s current location instead of window.location, since BrowserRouter
basename can make same-route checks miss and still trigger navigate. Update the
guard to use location from useLocation() inside the callback, and include
location in the callback’s dependency list so the check stays in sync with
router state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9cedfa1-3cbf-4a8d-bf13-95f2e3462b6f
📒 Files selected for processing (1)
frontend/packages/console-app/src/components/detect-context/useValuesForPerspectiveContext.ts
|
/retest |
|
@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. |
3 participants
Closes #16254
Analysis / Root cause:
setPerspective() always called navigate() unconditionally, causing redundant page reloads for custom perspective plugins. When the target URL matched the current location, this triggered namespace handler validation loops and visible re-renders.
This also stripped the ?perspective= param in DetectPerspective to prevent it from persisting and re-triggering the effect.
Solution description:
Skip navigate() when the target URL matches the current location
Screenshots / screen recording:
Test setup:
Refer to reproduction steps in linked issue
Test cases:
Ensure reproduction steps in linked issue are no longer reproducible
Summary by CodeRabbit
perspectivequery parameter from the URL when present, keeping the address bar clean.