TRT-2764: Resolved conflicts from #3721 - Fix search bar not completing search#3818
TRT-2764: Resolved conflicts from #3721 - Fix search bar not completing search#3818not-stbenjam wants to merge 10 commits into
Conversation
The requestSearch function in 11 table components mutated the filterModel object returned by useStableJSONQueryParam in place. This corrupted the hook's internal ref state: the intermediate re-render from setSearching(true) would detect the mutation via the defaultValue fallback, update the serialized cache, and then when the URL update arrived with the same content, the comparison found no change, so the useEffect never fired and fetchData was never called. Fix by creating a new object with spread syntax instead of mutating the existing filterModel. This ensures useStableJSONQueryParam sees a genuinely new reference on re-render, triggering the useEffect and completing the search. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n refresh Fix two issues with the quick search bar: 1. Pressing Enter or clicking the search icon when the same search term is already active no longer gets stuck in loading state. The requestSearch function in TestTable now checks if the filter would actually change before setting the searching state. 2. When refreshing the page, the quick search bar is now repopulated from the filterModel URL query parameter. GridToolbar accepts a new searchField prop that identifies which filter column corresponds to the search bar, and initializes the search text on mount. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two fixes in GridToolbar: 1. Remove onBlur handler from search TextField to prevent double-trigger when clicking the search icon (onBlur fires before onClick, causing two requestSearch calls that can race with state updates and leave isSearching stuck as true). 2. Only set initializedFromFilter ref when a search filter is actually found, so the search bar gets repopulated from URL params on page refresh even if the filterModel is initially empty on first render. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Only initialize the search bar from the URL filterModel when there is exactly one filter on the search field and it uses a positive 'contains' operator. This avoids misleadingly populating the search bar for NOT matchers or when multiple filters exist on the same column. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
ready-for-human-review
openshift-ci-robot
added
the
jira/valid-reference
|
@not-stbenjam: This pull request references TRT-2764 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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: not-stbenjam 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 |
WalkthroughGridToolbar now initializes from matching filters and avoids blur-triggered searches. DataGrid search handlers update filter models immutably, while toolbar configurations specify the corresponding searchable field across affected views. ChangesDataGrid search alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @not-stbenjam. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@sippy-ng/src/datagrid/GridToolbar.jsx`:
- Around line 42-64: Add regression tests for GridToolbar covering
initialization from a single contains filter, excluding negated or otherwise
unsupported filters, triggering searches via Enter and the search button, and
confirming blur does not trigger a search. Use the existing toolbar test
patterns and cover both the useEffect synchronization and user interaction
paths.
- Around line 42-64: Update the initialization logic in the useEffect to require
filter.not !== true alongside the existing contains operator and value checks
before calling setSearch, so negated contains filters do not populate the
positive quick-search field.
In `@sippy-ng/src/jobs/JobRunsTable.jsx`:
- Around line 464-474: Add regression coverage for field-specific toolbar
searches and repeated searches. In sippy-ng/src/jobs/JobRunsTable.jsx lines
464-474 and sippy-ng/src/jobs/JobTable.jsx lines 536-546, verify immutable
replacement of only the job/name filter; in
sippy-ng/src/repositories/RepositoriesTable.jsx lines 281-291 and
sippy-ng/src/tests/FeatureGates.jsx lines 213-225, verify repo/feature_gate
replacement. Test toolbar initialization targets the configured contains filter
at sippy-ng/src/jobs/JobRunsTable.jsx line 651, sippy-ng/src/jobs/JobTable.jsx
line 710, sippy-ng/src/repositories/RepositoriesTable.jsx line 330,
sippy-ng/src/tests/FeatureGates.jsx line 413, sippy-ng/src/tests/TestTable.jsx
line 1144, sippy-ng/src/component_readiness/RegressedTestsPanel.jsx line 376,
sippy-ng/src/component_readiness/TriagedRegressionTestList.jsx line 358, and
sippy-ng/src/component_readiness/TriagedRegressions.jsx line 410. In
sippy-ng/src/tests/TestTable.jsx lines 1014-1031, cover unchanged-search no-op,
immutable name-filter replacement, and the repeated-search path clearing
loading.
🪄 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: Pro Plus
Run ID: e027a149-2b2c-468b-9257-815dcef99809
📒 Files selected for processing (16)
sippy-ng/src/component_readiness/RegressedTestsPanel.jsxsippy-ng/src/component_readiness/TriagedRegressionTestList.jsxsippy-ng/src/component_readiness/TriagedRegressions.jsxsippy-ng/src/datagrid/GridToolbar.jsxsippy-ng/src/jobs/JobAnalysis.jsxsippy-ng/src/jobs/JobRunsTable.jsxsippy-ng/src/jobs/JobTable.jsxsippy-ng/src/releases/PayloadStreamTestFailures.jsxsippy-ng/src/releases/PayloadStreamsTable.jsxsippy-ng/src/releases/PayloadTestFailures.jsxsippy-ng/src/releases/ReleasePayloadJobRuns.jsxsippy-ng/src/releases/ReleasePayloadPullRequests.jsxsippy-ng/src/releases/ReleasePayloadTable.jsxsippy-ng/src/repositories/RepositoriesTable.jsxsippy-ng/src/tests/FeatureGates.jsxsippy-ng/src/tests/TestTable.jsx
🔴 REQUEST_CHANGESPanel Review — PR #3818: TRT-2764: Resolved conflicts from #3721 - Fix search bar not completing search Two confirmed functional bugs found that undermine the PR's stated fix. Both were independently identified by 4–5 of 7 specialist reviewers and verified with runtime reproducers. 🐛 Bugs Specialist — 2 BLOCKING
⚔️ Adversarial Specialist — 2 BLOCKING, 3 SUGGESTION, 1 NOTE
🔒 Security Specialist — No findingsChecked XSS via search input, 🏗️ Architecture Specialist — 2 BLOCKING, 2 SUGGESTION, 2 NOTE
🔄 Consistency Specialist — 2 BLOCKING, 1 SUGGESTION, 3 NOTE
🧪 QA Specialist — 3 BLOCKING, 3 SUGGESTION, 2 NOTE
📝 Writer Specialist — 1 NOTE
Panel SynthesisConfirmed Bugs (reproducer-verified)Bug 1:
Bug 2:
Required Actions
Optional Follow-ups
Stats
Generated by /deep-review |
- Fix npm audit failure: use --audit-level=high since react-router moderate CVEs have no v6 patch (fix requires v7 + React 18 migration) - Fix negated filter initialization: check filter.not !== true before populating quick-search from a contains filter - Add GridToolbar tests: initialization from filters, negated filter exclusion, Enter/button search triggering, blur not triggering search - Add JobRunsTable requestSearch tests: immutable filter replacement, preservation of unrelated filters Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest |
Another PR is fixing the react-router audit issue directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@sippy-ng/src/jobs/JobRunsTable.test.jsx`:
- Around line 10-23: Replace the duplicated requestSearch helper in
JobRunsTable.test.jsx with coverage of the production JobRunsTable search
implementation. Extract and import the shared helper if appropriate, or render
JobRunsTable and assert the resulting setFilterModel call, while preserving the
existing filtering and replacement behavior under test.
🪄 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: Pro Plus
Run ID: 17ac7834-6dff-455b-b382-5fd311387756
📒 Files selected for processing (4)
Makefilesippy-ng/src/datagrid/GridToolbar.jsxsippy-ng/src/datagrid/GridToolbar.test.jsxsippy-ng/src/jobs/JobRunsTable.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- sippy-ng/src/datagrid/GridToolbar.jsx
- JobAnalysis.jsx: Convert requestSearch to immutable pattern using spread operator so React detects the state change - ReleasePayloadJobRuns.jsx: Fix columnField from 'releaseTag' to 'release_tag' to match the filter removal and column definition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@sippy-ng/src/jobs/JobAnalysis.jsx`:
- Around line 264-271: Add regression tests for the search-filter update flow
around the testFilter state update: verify an existing name filter is replaced
with the new search criterion, while unrelated filters remain unchanged. Ensure
the assertions cover immutable state behavior without mutating the original
testFilter items.
🪄 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: Pro Plus
Run ID: 3c10769c-e0c6-4c67-84df-25e5c9c17dd6
📒 Files selected for processing (2)
sippy-ng/src/jobs/JobAnalysis.jsxsippy-ng/src/releases/ReleasePayloadJobRuns.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- sippy-ng/src/releases/ReleasePayloadJobRuns.jsx
|
/test agentic-staging |
Sippy Staging EnvironmentURL: https://quality-pills-surge-def.trycloudflare.com This environment is built from this PR and will remain available for approximately 60 minutes (until ~21:25 UTC). |
|
Scheduling required tests: |
|
/test agentic-staging |
Sippy Staging EnvironmentURL: https://img-favourite-alexander-friendly.trycloudflare.com This environment is built from this PR and will remain available for approximately 60 minutes (until ~21:57 UTC). |
|
@not-stbenjam: 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
Summary
Rebased and resolved merge conflicts from #3721 against the current
mainbranch.The original PR fixed the search bar on table pages (Tests, Jobs, etc.) not completing searches after pressing Enter. The URL would update with the search terms, but the table stayed in a loading/spinner state indefinitely.
Conflicts resolved
The primary source of conflicts was that
mainhad renamed all.jsfiles to.jsx(e.g.,TestTable.js→TestTable.jsx), while #3721 modified the original.jsfiles. Git's rename detection successfully auto-merged all 16 changed files:sippy-ng/src/tests/TestTable.jsx— immutablerequestSearch,searchFieldpropsippy-ng/src/tests/FeatureGates.jsx— immutablerequestSearchsippy-ng/src/jobs/JobTable.jsx— immutablerequestSearchsippy-ng/src/jobs/JobRunsTable.jsx— immutablerequestSearchsippy-ng/src/releases/PayloadStreamTestFailures.jsx— immutablerequestSearchsippy-ng/src/releases/PayloadStreamsTable.jsx— immutablerequestSearchsippy-ng/src/releases/PayloadTestFailures.jsx— immutablerequestSearchsippy-ng/src/releases/ReleasePayloadJobRuns.jsx— immutablerequestSearchsippy-ng/src/releases/ReleasePayloadPullRequests.jsx— immutablerequestSearchsippy-ng/src/releases/ReleasePayloadTable.jsx— immutablerequestSearchsippy-ng/src/repositories/RepositoriesTable.jsx— immutablerequestSearchsippy-ng/src/datagrid/GridToolbar.jsx—searchFieldprop,initializedFromFilterlogicsippy-ng/src/component_readiness/RegressedTestsPanel.jsx—searchFieldprop passthroughsippy-ng/src/component_readiness/TriagedRegressionTestList.jsx—searchFieldprop passthroughsippy-ng/src/component_readiness/TriagedRegressions.jsx—searchFieldprop passthroughsippy-ng/src/jobs/JobAnalysis.jsx—searchFieldprop passthroughOriginal PR commits (preserved)
061b4cb— Fix search bar not completing search until page refresh (immutablerequestSearchin 11 components)45601e8— Fix double-search stuck loading and repopulate search bar on refresh9ca19dd— Fix double-search stuck loading and search bar repopulation (removeonBlurhandler)db0562a— Only populate quick search from single positive filterValidation
Test plan
make lintpasses in CImake testpasses in CISupersedes #3721
Jira: https://issues.redhat.com/browse/TRT-2764
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests