OCPBUGS-85121: Skip redundant navigate() in setPerspective when target matches current URL#16410
OCPBUGS-85121: Skip redundant navigate() in setPerspective when target matches current URL#16410logonoff wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-85121, 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: logonoff 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
📝 WalkthroughWalkthroughThese changes refactor perspective synchronization logic in the console's perspective detection system. The 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 (4)
frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsx (1)
28-30: 💤 Low valueConsider using the
useLocation()return value forperspectiveParamcomputation.
getPerspectiveURLParamreadswindow.location.searchdirectly whileuseLocation()is called separately as a subscription trigger. These two sources are synchronised in a browser SPA context, so this is not a bug — but composing them more explicitly avoids any future divergence.♻️ Suggested refactor
- const perspectiveParam = getPerspectiveURLParam(perspectiveExtensions); - // Subscribe to location changes so perspectiveParam is recalculated on navigation - useLocation(); + const location = useLocation(); + const perspectiveParam = getPerspectiveURLParam(perspectiveExtensions, location.search);And update
getPerspectiveURLParamaccordingly:-const getPerspectiveURLParam = (perspectives: Perspective[]) => { - const perspectiveIDs = perspectives.map( - (nextPerspective: Perspective) => nextPerspective.properties.id, - ); - const urlParams = new URLSearchParams(window.location.search); +const getPerspectiveURLParam = (perspectives: Perspective[], search: string) => { + const perspectiveIDs = perspectives.map( + (nextPerspective: Perspective) => nextPerspective.properties.id, + ); + const urlParams = new URLSearchParams(search); const perspectiveParam = urlParams.get('perspective'); return perspectiveParam && perspectiveIDs.includes(perspectiveParam) ? perspectiveParam : ''; };🤖 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/DetectPerspective.tsx` around lines 28 - 30, The current code calls useLocation() only as a subscription trigger while getPerspectiveURLParam reads window.location.search directly; change this to derive perspectiveParam from the location object returned by useLocation() so the two are explicitly composed. Specifically, get the location via const location = useLocation(), pass either location or location.search into getPerspectiveURLParam (or update getPerspectiveURLParam to accept a search string), and compute perspectiveParam from that argument instead of reading window.location; update any call sites to the modified getPerspectiveURLParam signature (e.g., the perspectiveParam assignment) and ensure types are adjusted accordingly.frontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts (2)
24-24: 💤 Low value
latestPerspectivetyping —falsein the falsy chain is intentional, but warrants a note.
latestPerspectiveevaluates tofalsewhenloadedisfalse, becausefalse && (...)short-circuits. The expressionactivePerspective || latestPerspective || ''then correctly collapses it to''. This works, though an explicit(loaded && ...) || ''would make the intent clearer and avoid the implicitfalse-to-''coercion if the type signature ever widens.🤖 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` at line 24, The current expression uses activePerspective || latestPerspective || '' where latestPerspective can be false when loaded is false; change it to explicitly guard by loaded to avoid implicit false-to-empty-string coercion: replace the assignment to perspective with activePerspective || ((loaded && latestPerspective) || '') or equivalently (activePerspective || (loaded && latestPerspective) || '') so intent is clear and types remain correct (referencing variables perspective, activePerspective, latestPerspective, and loaded).
2-2: Consider usinguseLocation()for type-safe location handling instead ofwindow.location.While
window.locationand react-router'sLocationtype are structurally compatible (both exposepathname,search,hashas strings), they're maintained independently. The codebase consistently uses react-router'suseLocation()hook elsewhere (e.g.,PerspectiveDetector.tsx,useTabbedTableBreadcrumb.ts). UsinguseLocation()here would provide better type consistency and clarity about intent—whether the component needs the route location or the actual browser location. If actual browser location is intentional here, consider adding a comment explaining whywindow.locationis preferred overuseLocation().🤖 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` at line 2, The code imports createPath and useNavigate from 'react-router' but elsewhere in this file it uses window.location; replace usage of window.location with react-router's useLocation() for type-safe location handling and import useLocation from 'react-router' alongside createPath/useNavigate (or if the intent truly is to read the raw browser URL, add a clarifying comment where window.location is used). Update the hook usage in the component (e.g., call useLocation() and use its pathname/search/hash) and remove direct references to window.location to keep consistency with other modules like PerspectiveDetector.tsx and useTabbedTableBreadcrumb.ts.frontend/packages/console-app/src/components/detect-perspective/__tests__/useValuesForPerspectiveContext.spec.ts (1)
28-103: ⚡ Quick winAdd test coverage for the core navigation guard in
setPerspective.The central fix in this PR — skipping
navigate()whentarget === createPath(window.location)— has no unit test coverage at all. Without it, a regression is invisible. Consider adding at least two cases usingjest.spyOnor mockinguseNavigate:
- When
targetequals the current path →navigateis NOT called.- When
targetdiffers from the current path →navigateIS called with the correct value.🤖 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__/useValuesForPerspectiveContext.spec.ts` around lines 28 - 103, Add unit tests covering the navigation guard in setPerspective: write one test that mocks/window.location and createPath to return the same string as the target and spies on useNavigate/navigate to assert navigate is NOT called, and a second test where createPath(window.location) differs from target and assert navigate IS called with the expected path; reference setPerspective, createPath, and useNavigate/navigate when locating the logic to test and use jest.spyOn or a useNavigate mock to observe calls.
🤖 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 32-35: The current code sets target = next || '/' which navigates
to '/' when next is undefined; change the default so missing next uses the
current location instead: set target to next ?? createPath(window.location) (or
use next ? next : createPath(window.location)) so the existing guard (if (target
!== createPath(window.location)) navigate(target)) will no-op when callers omit
next; update the code around the target assignment in
useValuesForPerspectiveContext to use createPath(window.location) as the
fallback rather than '/'.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/detect-perspective/__tests__/useValuesForPerspectiveContext.spec.ts`:
- Around line 28-103: Add unit tests covering the navigation guard in
setPerspective: write one test that mocks/window.location and createPath to
return the same string as the target and spies on useNavigate/navigate to assert
navigate is NOT called, and a second test where createPath(window.location)
differs from target and assert navigate IS called with the expected path;
reference setPerspective, createPath, and useNavigate/navigate when locating the
logic to test and use jest.spyOn or a useNavigate mock to observe calls.
In
`@frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsx`:
- Around line 28-30: The current code calls useLocation() only as a subscription
trigger while getPerspectiveURLParam reads window.location.search directly;
change this to derive perspectiveParam from the location object returned by
useLocation() so the two are explicitly composed. Specifically, get the location
via const location = useLocation(), pass either location or location.search into
getPerspectiveURLParam (or update getPerspectiveURLParam to accept a search
string), and compute perspectiveParam from that argument instead of reading
window.location; update any call sites to the modified getPerspectiveURLParam
signature (e.g., the perspectiveParam assignment) and ensure types are adjusted
accordingly.
In
`@frontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts`:
- Line 24: The current expression uses activePerspective || latestPerspective ||
'' where latestPerspective can be false when loaded is false; change it to
explicitly guard by loaded to avoid implicit false-to-empty-string coercion:
replace the assignment to perspective with activePerspective || ((loaded &&
latestPerspective) || '') or equivalently (activePerspective || (loaded &&
latestPerspective) || '') so intent is clear and types remain correct
(referencing variables perspective, activePerspective, latestPerspective, and
loaded).
- Line 2: The code imports createPath and useNavigate from 'react-router' but
elsewhere in this file it uses window.location; replace usage of window.location
with react-router's useLocation() for type-safe location handling and import
useLocation from 'react-router' alongside createPath/useNavigate (or if the
intent truly is to read the raw browser URL, add a clarifying comment where
window.location is used). Update the hook usage in the component (e.g., call
useLocation() and use its pathname/search/hash) and remove direct references to
window.location to keep consistency with other modules like
PerspectiveDetector.tsx and useTabbedTableBreadcrumb.ts.
🪄 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: 687f30ba-a6a9-47a4-a20a-5736009b28c9
📒 Files selected for processing (3)
frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsxfrontend/packages/console-app/src/components/detect-perspective/__tests__/useValuesForPerspectiveContext.spec.tsfrontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts
📜 Review details
🔇 Additional comments (1)
frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsx (1)
31-39: LGTM — the?perspective=cleanup and loop-prevention logic are correct.Stripping the param before passing it as
targetensures the one-shotuseEffectfire doesn't create a navigation loop: afternavigate(target)the new URL lacks?perspective=, soperspectiveParamevaluates to''on the next render and the guardperspectiveParam && perspectiveParam !== activePerspectiveis false.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: 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
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. |
logonoff
force-pushed
the
OCPBUGS-85121-router-perspective
branch
6 times, most recently
from
33b99a5 to
474fc47
Compare
|
@logonoff: 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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-perspective/DetectPerspective.tsx`:
- Around line 31-39: The code currently only removes the perspective query when
perspectiveParam !== activePerspective; change the logic to always clear the
'perspective' query when perspectiveParam is present, but only call
setActivePerspective(perspectiveParam, ...) if it differs from
activePerspective. In DetectPerspective.tsx, detect perspectiveParam, construct
params via new URLSearchParams(location.search), params.delete('perspective'),
build search and path with createPath({ ...location, search: search ?
`?${search}` : '' }), and then call setActivePerspective only when
perspectiveParam !== activePerspective; this ensures the ?perspective= param is
removed in all cases while preserving the existing behavior of
setActivePerspective.
🪄 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: 3271d260-c303-41b3-9729-7bfb83927a07
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/detect-perspective/DetectPerspective.tsxfrontend/packages/console-app/src/components/detect-perspective/useValuesForPerspectiveContext.ts
…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 DetectPerspective to prevent it from persisting and re-triggering the effect. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logonoff
force-pushed
the
OCPBUGS-85121-router-perspective
branch
from
474fc47 to
bbd3ece
Compare
|
@logonoff: The following tests 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. |
2 participants
Closes #16254
Analysis / Root cause:
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 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