OCPBUGS-81519: Fix Search page state mutation and unnecessary component remounts#16266
OCPBUGS-81519: Fix Search page state mutation and unnecessary component remounts#16266stefanonardo wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-81519, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stefanonardo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-81519, 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 pull request introduces URL↔filter state synchronization in the data view filter hook and refactors the search component's filtering logic. The hook now uses a ref-based side effect to detect and reconcile mismatches between internal filter state and URL query parameters, applying partial updates when divergence is detected. The search component consolidates filtering behavior around debounced name filters, memoizes selector computation, mutes ResourceList with explicit props to prevent unnecessary reconciliation, and ensures immutability of selection state by cloning the Set before mutations. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/search.tsx (1)
136-146:⚠️ Potential issue | 🟡 MinorHydrate the debounced name state directly from the URL.
Line 136 only updates
typeaheadNameFilter. On back/forward navigation or any externallocation.searchchange,debouncedNameFilterstays stale until the 300ms callback fires, so the input/URL and rendered results can disagree for one debounce window.Suggested fix
- setTypeaheadNameFilter(name || ''); + const nextName = name || ''; + setTypeaheadNameFilter(nextName); + setDebouncedNameFilter(nextName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/search.tsx` around lines 136 - 146, When hydrating from location.search you only update typeaheadNameFilter, leaving debouncedNameFilter stale until the debounce fires; update debouncedNameFilter immediately as well to keep UI/results in sync. In the effect that reads the name from location.search (where setTypeaheadNameFilter(name || '') is called), also call setDebouncedNameFilter(name || '') so the debounced state mirrors the URL instantly without invoking debouncedNameFilterCallback (which would re-trigger setQueryArgument).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/public/components/search.tsx`:
- Around line 136-146: When hydrating from location.search you only update
typeaheadNameFilter, leaving debouncedNameFilter stale until the debounce fires;
update debouncedNameFilter immediately as well to keep UI/results in sync. In
the effect that reads the name from location.search (where
setTypeaheadNameFilter(name || '') is called), also call
setDebouncedNameFilter(name || '') so the debounced state mirrors the URL
instantly without invoking debouncedNameFilterCallback (which would re-trigger
setQueryArgument).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c5d56cc9-28dc-474d-81a4-8b0c6d7d3aa7
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/data-view/useConsoleDataViewFilters.tsfrontend/public/components/search.tsx
📜 Review details
🧰 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/packages/console-app/src/components/data-view/useConsoleDataViewFilters.tsfrontend/public/components/search.tsx
🔇 Additional comments (1)
frontend/public/components/search.tsx (1)
148-163: [Rewritten comment]
[Exactly ONE classification tag]
|
/retest-required |
…nt remounts Fix Set reference copy bug in resource selection handlers, replace filter-embedded component keys with stable keys to prevent WebSocket watch teardown, and add memo/debounce to ResourceList to avoid expensive re-renders during typing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b5bd9af to
9784246
Compare
|
/retest-required |
|
@stefanonardo: all tests passed! 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. |
Summary
updateSelectedItemsandupdateNewItems— creates a newSetinstead of copying the reference, ensuring React detects state changeskey={resource}to preventResourceListfrom remounting on every filter change (which tore down WebSocket watches and caused duplicate LIST requests)ResourceListinReact.memoand debounce thenameFilterprop + URL update to prevent expensive re-renders during typingselectorprop withuseMemofor referential stability withReact.memouseConsoleDataViewFiltersto keep filters in sync without remounting (compensates for PatternFly'suseDataViewFiltersonly readingsearchParamson mount)Note on PatternFly upstream
PatternFly's
useDataViewFiltershook only readssearchParamson mount (empty depsuseEffect), which means it assumes the component remounts when URL filters change. This forced us to add a workarounduseEffectinuseConsoleDataViewFiltersto sync URL changes to internal filter state. Ideally,useDataViewFiltersshould be reactive tosearchParamschanges natively — an upstream fix in@patternfly/react-data-viewwould allow us to remove this workaround.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements