OCPBUGS-63391: Replace DataViewTextFilter with existing custom TextFilter#16151
OCPBUGS-63391: Replace DataViewTextFilter with existing custom TextFilter#16151rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@rhamilto: This pull request references Jira Issue OCPBUGS-63391, 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. |
openshift-ci
bot
added
component/core
|
/jira refresh |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
jira/valid-bug
|
@rhamilto: This pull request references Jira Issue OCPBUGS-63391, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@rhamilto: This pull request references Jira Issue OCPBUGS-63391, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/assign @vojtechszocs |
📝 WalkthroughWalkthroughThis change extracts the DataViewTextFilter component into a dedicated local module. A new file introduces the DataViewTextFilter component, which wraps PatternFly's ToolbarFilter and TextFilter components. The component accepts configuration props (title, filterId, placeholder, onChange, showToolbarItem) and manages filter state by reading from URL search parameters while maintaining local input state. The ConsoleDataView component's import is updated to reference the new local module instead of the PatternFly library source. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-app/src/components/data-view/DataViewTextFilter.tsx`:
- Around line 21-23: The input clearing bug is caused by rendering the field as
inputText || filterValue and not initializing/syncing local state from the URL;
update the component to initialize inputText from searchParams (useSearchParams)
on mount and stop using the fallback (remove uses of `inputText ||
filterValue`), and add a useEffect that sets
setInputText(searchParams.get(filterId) ?? '') when searchParams or filterId
change so the local state is the single source of truth while still reacting to
external URL navigation; adjust any usage in the component that reads
`filterValue` (e.g., in the input value and change handlers) to rely on
inputText and setInputText.
- Line 1: Add the React namespace import and update the event handler type where
React.FormEvent is used in DataViewTextFilter.tsx: import the React symbol (e.g.
import React from 'react' or import * as React from 'react') at the top of the
file, then update the handler signature that currently references
React.FormEvent (the function around line 25) to use the proper generic for an
input element (e.g. React.FormEvent<HTMLInputElement> or
React.ChangeEvent<HTMLInputElement>) so the type resolves correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9a7b173a-7747-40c2-84cc-38495b4d69cd
📒 Files selected for processing (2)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsxfrontend/packages/console-app/src/components/data-view/DataViewTextFilter.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/DataViewTextFilter.tsxfrontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx
🧬 Code graph analysis (1)
frontend/packages/console-app/src/components/data-view/DataViewTextFilter.tsx (1)
frontend/public/components/utils/tile-view-page.jsx (1)
searchParams(466-466)
🔇 Additional comments (1)
frontend/packages/console-app/src/components/data-view/ConsoleDataView.tsx (1)
28-28: Import switch to local wrapper looks correct.The change from PatternFly's
DataViewTextFilterto the local implementation maintains the same component interface and integrates cleanly with the existingDataViewFilterscontext. This preserves the established filtering architecture while enabling the custom behavior.
| @@ -0,0 +1,40 @@ | |||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
Missing React namespace import for type annotation.
React.FormEvent is used on line 25, but only useState is imported from 'react'. Depending on your TypeScript configuration, this may cause a compilation error.
🔧 Proposed fix
-import { useState } from 'react';
+import type { FormEvent } from 'react';
+import { useState } from 'react';
import { ToolbarFilter } from '@patternfly/react-core';
import { useSearchParams } from 'react-router';
import { TextFilter } from '@console/internal/components/factory/text-filter';Then update the handler signature:
- const handleChange = (_event: React.FormEvent<HTMLInputElement>, value: string) => {
+ const handleChange = (_event: FormEvent<HTMLInputElement>, value: string) => {Also applies to: 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-app/src/components/data-view/DataViewTextFilter.tsx`
at line 1, Add the React namespace import and update the event handler type
where React.FormEvent is used in DataViewTextFilter.tsx: import the React symbol
(e.g. import React from 'react' or import * as React from 'react') at the top of
the file, then update the handler signature that currently references
React.FormEvent (the function around line 25) to use the proper generic for an
input element (e.g. React.FormEvent<HTMLInputElement> or
React.ChangeEvent<HTMLInputElement>) so the type resolves correctly.
| const [searchParams] = useSearchParams(); | ||
| const [inputText, setInputText] = useState(''); | ||
| const filterValue = searchParams.get(filterId) ?? ''; |
There was a problem hiding this comment.
State synchronization bug prevents clearing the filter when URL params exist.
The pattern inputText || filterValue has a subtle but impactful issue: when a user clears the input, inputText becomes '' (falsy), causing the component to fall back to displaying filterValue from the URL. This makes it impossible to clear the filter field when URL params are present.
Consider initializing inputText from the URL param on mount, then treating local state as the source of truth:
🐛 Proposed fix
export const DataViewTextFilter = ({
title,
filterId,
placeholder,
onChange,
showToolbarItem,
}: DataViewTextFilterProps) => {
const [searchParams] = useSearchParams();
- const [inputText, setInputText] = useState('');
- const filterValue = searchParams.get(filterId) ?? '';
+ const [inputText, setInputText] = useState(() => searchParams.get(filterId) ?? '');
const handleChange = (_event: React.FormEvent<HTMLInputElement>, value: string) => {
setInputText(value);
onChange?.(filterId, value);
};
return (
<ToolbarFilter categoryName={title} showToolbarItem={showToolbarItem}>
<TextFilter
label={filterId}
placeholder={placeholder}
- value={inputText || filterValue}
+ value={inputText}
onChange={handleChange}
/>
</ToolbarFilter>
);
};If the component should also react to external URL changes (browser back/forward navigation), you'd need an effect to sync:
useEffect(() => {
setInputText(searchParams.get(filterId) ?? '');
}, [searchParams, filterId]);Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-app/src/components/data-view/DataViewTextFilter.tsx`
around lines 21 - 23, The input clearing bug is caused by rendering the field as
inputText || filterValue and not initializing/syncing local state from the URL;
update the component to initialize inputText from searchParams (useSearchParams)
on mount and stop using the fallback (remove uses of `inputText ||
filterValue`), and add a useEffect that sets
setInputText(searchParams.get(filterId) ?? '') when searchParams or filterId
change so the local state is the single source of truth while still reacting to
external URL navigation; adjust any usage in the component that reads
`filterValue` (e.g., in the input value and change handlers) to rely on
inputText and setInputText.
|
/hold This broke the filter chip |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
openshift-ci
bot
added
the
kind/cypress
rhamilto
changed the title
rhamilto
changed the title
|
@rhamilto: This pull request references Jira Issue OCPBUGS-63391, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
There was a problem hiding this comment.
Rules are no longer needed
There was a problem hiding this comment.
Rules are orphaned (first block) or no longer needed (second block).
|
/verified by @XiyuZhao |
openshift-ci-robot
added
the
verified
|
@XiyunZhao: This PR has been marked as verified by 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: rhamilto, 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 |
I think you tagged me by accident. Unsubscribing from this thread now. |
|
/retest |
|
@rhamilto: 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. |
9 participants
Description
This PR replaces PatternFly's
DataViewTextFilterwith the legacyTextFiltercomponent into the DataView filter system in order to re-enable the keyboard shortcut to the name text input.After
Screen.Recording.2026-03-16.at.12.15.46.PM.mov
Changes
DataViewTextFilter.tsxwrapper component that wrapsTextFilterinToolbarFilterDataViewTextFilterimport fromConsoleDataView.tsxBenefits
TextFilter)Testing
Summary by CodeRabbit