OU-1237: 4.22 breaking changes#368
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1237 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 task 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. |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces legacy React namespace usage with named React imports and FC typings across frontend code, upgrades toolchain/config (TypeScript, webpack, ESLint, packages), removes a multi-stage Dockerfile, updates dev Docker/Make/devspace settings, and applies broad Cypress test/support formatting and small test wiring edits. ChangesReact & Tooling Modernization
Cypress Tests & Support
Build / Docker / Devspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/cypress/e2e/integration/logs-page.cy.ts (1)
103-103: ⚡ Quick winReplace fixed sleep with a deterministic wait
Line 103 uses a hard
cy.wait(500), which can still flake under slower CI and adds avoidable latency. Since this test already aliases the initial request, wait on that alias instead.Proposed change
- cy.visit(LOGS_PAGE_URL).wait(500); + cy.visit(LOGS_PAGE_URL); + cy.wait('@queryRangeStreams');🤖 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 `@web/cypress/e2e/integration/logs-page.cy.ts` at line 103, Replace the fixed sleep after cy.visit(LOGS_PAGE_URL).wait(500) with a deterministic wait on the previously created network alias—remove the .wait(500) and call cy.wait('@<theAliasName>') (use the actual alias used in this test) immediately after cy.visit(LOGS_PAGE_URL) so the test waits for the real request to complete instead of sleeping.web/src/hooks/useURLState.ts (2)
208-234: 💤 Low valuePotential stale closure in URL normalization effect.
setQueryInURLcapturesqueryParamsandlocationfrom the closure at render time. Since these are not in the dependency array (and the eslint rule is disabled), the effect may use stale values if the URL changes between renders.Consider wrapping
setQueryInURLinuseCallbackwith proper dependencies, or includequeryParamsin this effect's dependency array to ensure freshness.🤖 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 `@web/src/hooks/useURLState.ts` around lines 208 - 234, The URL-normalization useEffect is susceptible to a stale closure because setQueryInURL (which references queryParams and location) can capture outdated values; update the code so setQueryInURL is stable or the effect sees fresh values—either memoize setQueryInURL with useCallback including queryParams and location in its dependency list, or add queryParams (and location if used) to the effect dependencies that currently include initialSchema, schema, and configLoaded; ensure you keep getDefaultQuery/defaultQueryFromTenant and the logic comparing defaultOtelQuery/defaultViaqQuery unchanged while removing the eslint-disable so React's lint enforces correct deps.
69-69: ⚡ Quick winConsider using
setSearchParamsfor cleaner URL updates.The code retrieves
queryParamsfromuseSearchParamsbut ignores the setter function, instead manually constructing URL strings withnavigate(). UsingsetSearchParamswould simplify the URL update logic throughout this file.Example for
setQueryInURL:const [queryParams, setSearchParams] = useSearchParams(); // ... const setQueryInURL = (newQuery: string, replace?: boolean) => { const trimmedQuery = newQuery.trim(); setSearchParams((prev) => { prev.set(QUERY_PARAM_KEY, trimmedQuery); return prev; }, { replace }); };🤖 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 `@web/src/hooks/useURLState.ts` at line 69, The hook currently calls useSearchParams only to read queryParams and manually builds URLs with navigate, which is more error-prone; update the hook to destructure both values from useSearchParams (const [queryParams, setSearchParams] = useSearchParams()) and refactor functions that modify the URL—notably setQueryInURL and any navigate-based setters that change QUERY_PARAM_KEY—to call setSearchParams with an updater callback (mutating/setting QUERY_PARAM_KEY on the passed URLSearchParams) and pass the replace option when needed, removing manual URL string construction and navigate usage for query-only changes.
🤖 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.
Nitpick comments:
In `@web/cypress/e2e/integration/logs-page.cy.ts`:
- Line 103: Replace the fixed sleep after cy.visit(LOGS_PAGE_URL).wait(500) with
a deterministic wait on the previously created network alias—remove the
.wait(500) and call cy.wait('@<theAliasName>') (use the actual alias used in
this test) immediately after cy.visit(LOGS_PAGE_URL) so the test waits for the
real request to complete instead of sleeping.
In `@web/src/hooks/useURLState.ts`:
- Around line 208-234: The URL-normalization useEffect is susceptible to a stale
closure because setQueryInURL (which references queryParams and location) can
capture outdated values; update the code so setQueryInURL is stable or the
effect sees fresh values—either memoize setQueryInURL with useCallback including
queryParams and location in its dependency list, or add queryParams (and
location if used) to the effect dependencies that currently include
initialSchema, schema, and configLoaded; ensure you keep
getDefaultQuery/defaultQueryFromTenant and the logic comparing
defaultOtelQuery/defaultViaqQuery unchanged while removing the eslint-disable so
React's lint enforces correct deps.
- Line 69: The hook currently calls useSearchParams only to read queryParams and
manually builds URLs with navigate, which is more error-prone; update the hook
to destructure both values from useSearchParams (const [queryParams,
setSearchParams] = useSearchParams()) and refactor functions that modify the
URL—notably setQueryInURL and any navigate-based setters that change
QUERY_PARAM_KEY—to call setSearchParams with an updater callback
(mutating/setting QUERY_PARAM_KEY on the passed URLSearchParams) and pass the
replace option when needed, removing manual URL string construction and navigate
usage for query-only changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b1a3c509-2a07-40a3-9925-5ea3bb45732c
📒 Files selected for processing (5)
web/cypress/e2e/integration/logs-alerts.cy.tsweb/cypress/e2e/integration/logs-detail-page.cy.tsweb/cypress/e2e/integration/logs-dev-page.cy.tsweb/cypress/e2e/integration/logs-page.cy.tsweb/src/hooks/useURLState.ts
✅ Files skipped from review due to trivial changes (1)
- web/cypress/e2e/integration/logs-alerts.cy.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/components/tenant-dropdown.tsx (2)
40-47: ⚡ Quick winGuard the missing-value case before normalizing the selection.
PatternFly types
Select.onSelectwith an optionalvalue. If that ever comes through asundefined,String(value)becomes the literal"undefined"and this code will forward it as if it were a real tenant. An earlyvalue == nullreturn makes the handler robust. (pf-react-staging.patternfly.org)Suggested change
const onSelect = ( _: ReactMouseEvent<Element, MouseEvent> | undefined, value: string | number | undefined, ) => { + if (value == null) { + setIsOpen(false); + return; + } const selectedValue = isOption(value) ? value.value : String(value); setIsOpen(false); if (selectedValue && selectedValue !== selectedTenant) { onTenantSelected?.(selectedValue); }🤖 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 `@web/src/components/tenant-dropdown.tsx` around lines 40 - 47, The onSelect handler currently normalizes an optional value with String(value) which can produce the literal "undefined" and be forwarded as a tenant; update the onSelect function to early-return when value == null (or value === undefined) before calling isOption/String, so you only proceed to setIsOpen, compare against selectedTenant, and call onTenantSelected with a real selection; reference the onSelect function and use the existing helpers isOption, setIsOpen, selectedTenant, and onTenantSelected when implementing the guard.
12-19: ⚡ Quick winLet
onOpenChangeown outside-close behavior.PatternFly already calls
onOpenChangewhen the menu should close because of outside clicks or configured close keys. Keeping a second window-level click listener here duplicates that logic and gives this component two separate close paths to maintain. (pf-react-staging.patternfly.org)Suggested change
import { FC, MouseEvent as ReactMouseEvent, Ref, - useCallback, - useEffect, - useRef, useState, } from 'react'; @@ - const selectRef = useRef<HTMLDivElement>(null); - const onToggle = () => setIsOpen(!isOpen); @@ - const handleClickOutside = useCallback( - (event: MouseEvent) => { - if (isOpen && !selectRef.current?.contains(event.target as Node)) { - setIsOpen(false); - } - }, - [isOpen, setIsOpen], - ); - - useEffect(() => { - if (isOpen) { - window.addEventListener('click', handleClickOutside); - } - return () => { - window.removeEventListener('click', handleClickOutside); - }; - }, [isOpen, handleClickOutside]); @@ - <div ref={selectRef}> + <div>Also applies to: 37-38, 51-67, 82-82
🤖 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 `@web/src/components/tenant-dropdown.tsx` around lines 12 - 19, Remove the manual window-level click listener and associated outside-click handling so PatternFly's onOpenChange controls outside-close behavior; specifically, delete the useEffect that adds/removes window.addEventListener('click', ...) and any helper like handleClickOutside or logic that calls setIsOpen(false) from that listener, and instead rely on the component's onOpenChange/onToggle handlers (e.g., the isOpen state setter and the onOpenChange prop passed into TenantDropdown) so there is a single close path managed by PatternFly.
🤖 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.
Nitpick comments:
In `@web/src/components/tenant-dropdown.tsx`:
- Around line 40-47: The onSelect handler currently normalizes an optional value
with String(value) which can produce the literal "undefined" and be forwarded as
a tenant; update the onSelect function to early-return when value == null (or
value === undefined) before calling isOption/String, so you only proceed to
setIsOpen, compare against selectedTenant, and call onTenantSelected with a real
selection; reference the onSelect function and use the existing helpers
isOption, setIsOpen, selectedTenant, and onTenantSelected when implementing the
guard.
- Around line 12-19: Remove the manual window-level click listener and
associated outside-click handling so PatternFly's onOpenChange controls
outside-close behavior; specifically, delete the useEffect that adds/removes
window.addEventListener('click', ...) and any helper like handleClickOutside or
logic that calls setIsOpen(false) from that listener, and instead rely on the
component's onOpenChange/onToggle handlers (e.g., the isOpen state setter and
the onOpenChange prop passed into TenantDropdown) so there is a single close
path managed by PatternFly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0fb1406e-b834-4f42-af8f-2ebc31a35c99
📒 Files selected for processing (1)
web/src/components/tenant-dropdown.tsx
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
/test ci/prow/test-e2e |
|
/retest |
|
@PeterYurkovich: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich, zhuje 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 |
|
/cherry-pick release-coo-ocp-4.22 |
|
@PeterYurkovich: #368 failed to apply on top of branch "release-coo-ocp-4.22": 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. |
5 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores