-
Notifications
You must be signed in to change notification settings - Fork 666
OCPBUGS-63401: Fix Pod selector links not navigating on Search page #15770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@sg00dwin: This pull request references Jira Issue OCPBUGS-63401, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
WalkthroughThe search component is refactored to use react-router-dom's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/public/components/search.tsx (1)
131-146: Avoid mutatingselectedItemsSetin place to ensure reliable re-rendersBoth
updateSelectedItemsandupdateNewItemsmutate the existingselectedItemsSetand then pass the same reference back intosetSelectedItems. Because React state updates can bail out when the new value is referentially equal, this pattern risks missed re-renders and makes behavior harder to reason about.Consider updating via a fresh
Setinstead:- const updateSelectedItems = (selection: string) => { - const updateItems = selectedItems; + const updateSelectedItems = (selection: string) => { + const updateItems = new Set(selectedItems); fireTelemetryEvent('search-resource-selected', { resource: selection, }); updateItems.has(selection) ? updateItems.delete(selection) : updateItems.add(selection); setSelectedItems(updateItems); setQueryArgument('kind', [...updateItems].join(',')); }; const updateNewItems = (_filter: string, { key }: ToolbarLabel) => { - const updateItems = selectedItems; + const updateItems = new Set(selectedItems); updateItems.has(key) ? updateItems.delete(key) : updateItems.add(key); setSelectedItems(updateItems); setQueryArgument('kind', [...updateItems].join(',')); };This keeps state updates immutable and avoids subtle UI sync issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/public/components/search.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/search.tsx
🧬 Code graph analysis (1)
frontend/public/components/search.tsx (3)
frontend/public/components/RBAC/role.jsx (2)
location(344-344)location(374-374)frontend/packages/console-shared/src/hooks/useLocation.ts (1)
useLocation(4-5)frontend/public/components/service-account.jsx (1)
kind(23-23)
🔇 Additional comments (2)
frontend/public/components/search.tsx (2)
6-6: Router-baseduseLocation+ effect dependency correctly address stale query param stateSwitching from
window.locationtoreact-router-dom’suseLocationand keying the effect offlocation.searchshould ensure the search page reacts when only the query string changes (e.g., pod selector links updating?kind=...), which aligns with SPA routing and fixes the original bug.One thing to double-check:
setQueryArgumentmust update the same history source thatreact-routeris listening to (not justwindow.locationdirectly), otherwise changes made viasetQueryArgumentmight not always trigger a newlocation.searchand thus won’t re-run this effect.Also applies to: 98-105, 121-121
114-116: Explicitly clearingselectedItemswhenkindis absent keeps URL and UI in syncAdding the
elsebranch to resetselectedItemsto an emptySetwhenkindisn’t present in the URL avoids stale selections and makes the empty-query state consistent with the “No resources selected” empty state below. This looks correct and aligned with the page behavior.
|
/retest-required |
rhamilto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Nice work, @sgoodwin!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, sg00dwin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@sg00dwin: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Clicking on the pod selector link was not displaying the associated pods in the search list.