CONSOLE-4402, CONSOLE-4510: React 18 upgrade - phase 1 #14869
Conversation
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
component/core
logonoff
force-pushed
the
react18
branch
2 times, most recently
from
c985555 to
b757ff6
Compare
openshift-ci
bot
added
component/gitops
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
the
plugin-api-changed
|
can we check the conflicts with base branch |
openshift-merge-robot
added
the
needs-rebase
- `firehose.spec.tsx` will use legacy render function because we expect to remove it before adopting the new render function (CONSOLE-4511)
- Updated `modulePathIgnorePatterns` and `testPathIgnorePatterns` to speed up test init due to conflicting module names (from Jest parsing files in the dist directory)
- Mock `TextEncoder` and `TextDecoder` due to jest-environment-jsdom issue
- Get rid of duplicate `jsdom` package by removing unused dep `browser-env`
- Deprecate testHook because RTL comes with renderHook
- Claude code fixed the remaining of the unit tests (changes seem reasonable enough)
AI slop explanation for mock in useUserSettings:
**Summary of fixes for useUserSettings.spec.ts:**
The two remaining failing tests with `sync=true` were fixed by simulating the server update after `setSettings()` is called. With `sync=true`, the hook prioritizes server data (cfData) over local state. When `updateConfigMap` resolves immediately (in mocked tests), the hook returns the cfData value. The fix simulates the k8s watcher reflecting the new value after the update:
```typescript
// With sync=true, the hook returns the value from cfData after the request completes.
// Simulate the server update by updating the mock to return the new value.
await act(async () => {
useK8sWatchResourceMock.mockReturnValue([
{ ...emptyConfigMap, data: { 'console.key': 'new value' } },
true,
null,
]);
rerender();
});
```
This matches the real-world behavior where the k8s watcher would receive the updated ConfigMap after the update succeeds.
openshift-ci-robot
removed
the
verified
|
@coderabbitai review |
|
Docs change /verified by @yapei |
✅ Actions performedReview triggered.
|
openshift-ci-robot
added
the
verified
|
@logonoff: 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. |
✅ Actions performedReview triggered.
|
|
@logonoff: This pull request references CONSOLE-4402 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 story to target the "4.22.0" version, but no target version was set. This pull request references CONSOLE-4510 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 story to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@dynamic-demo-plugin/package.json`:
- Line 23: The project currently pins react-router 5.3.4 and
react-router-dom-v5-compat alongside React 18 which is incompatible; update
package.json to replace "react-router" 5.3.4 and "react-router-dom-v5-compat"
with "react-router" (v6) and "react-router-dom" (v6), then refactor routing
usage: replace Switch with Routes, convert Route's component/render props to
element={...}, replace withRouter HOCs with hooks (useNavigate, useLocation,
useParams), and audit any route-related imports/usages (Switch, Route,
withRouter, history-based APIs) to their v6 equivalents to ensure runtime
compatibility with React 18.
In `@frontend/package.json`:
- Around line 205-208: react-dnd@11.1.3 and react-dnd-html5-backend@11.1.3 are
not officially compatible with React 18 (your package.json lists "react":
"^18.3.1"), so either verify DnD hooks/components for stability across React 18
or upgrade the DnD libraries: update "react-dnd" and "react-dnd-html5-backend"
to a React-18-compatible major (e.g., react-dnd 16+), run the workspace
install/build/tests to ensure hoisting still resolves to "react": "^18.3.1", and
fix any breaking API changes in your DnD usage (components/hooks referencing
react-dnd and react-dnd-html5-backend) accordingly.
In `@frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts`:
- Around line 14-15: The upgrade docs are missing migration guidance for the
breaking change where catalogDescription's type changed from ReactNode to
ComponentType (symbol: catalogDescription in console.catalog/item-type); update
upgrade-sdk.md for v4.22 to add clear instructions: explain that pre-rendered
React nodes must be wrapped in a ComponentType (e.g., a functional component
that returns the previous node), show the pattern to accept props if needed, and
provide an example conversion and note on importing ComponentType from React;
reference catalogDescription and the CHANGELOG entry CONSOLE-4402 for context.
In `@frontend/packages/console-shared/src/utils/pod-ring-utils.ts`:
- Around line 7-8: The code imports and uses the deprecated useSafetyFirst hook;
replace it with React's built-in useState by removing the useSafetyFirst import
and adding "import { useState } from 'react'"; then update all usages of
useSafetyFirst (e.g., any local variables or calls named useSafetyFirst) to
useState with identical initial values and consumers so behavior remains the
same; ensure the ImpersonateKind type import stays unchanged and run affected
tests/lint to catch any typing adjustments.
In
`@frontend/packages/dev-console/src/components/catalog/providers/__tests__/useDevfileSamples.spec.ts`:
- Line 1: Update the test to import React's native act by changing the act
import source: replace the current import of the act symbol from
'@testing-library/react' with an import from 'react' (i.e., import { act } from
'react') in the useDevfileSamples.spec.ts test file so the test uses React 18's
built-in act implementation.
In `@frontend/packages/knative-plugin/src/utils/create-channel-utils.ts`:
- Line 8: Remove the deprecated internal hook import useSafetyFirst and replace
calls with React's useState: delete the import line "useSafetyFirst" from
'@console/dynamic-plugin-sdk/src/app/components/safety-first' and import
useState from 'react' (or use React.useState where appropriate), then update
every usage of useSafetyFirst (e.g., any occurrences like const [foo, setFoo] =
useSafetyFirst(...)) to the equivalent useState initialization and setter (const
[foo, setFoo] = useState(...)). Ensure the semantics (initial value and types)
remain identical and remove any other references to safety-first.tsx per the
deprecation guidance.
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/components/actions/menu/ActionMenu.tsx (1)
5-6: Avoid deep import from SDK internals; expose a stable public API instead.Importing
useSafetyFirstfrom@console/dynamic-plugin-sdk/src/...couples this package to SDK internals and can break with packageexportsor build outputs. Prefer re‑exporting the hook from the SDK’s public barrel (or moving it into console‑shared) and importing from@console/dynamic-plugin-sdkonly. Please also verify the SDK package exports/tsconfig paths allow this deep import in production builds.frontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.md (1)
767-767: Minor grammar nit: consider hyphenating compound modifiers.Static analysis suggests "top level link" → "top-level link" for consistency with compound adjective rules. Similarly, line 243 has "type specific catalog" which could be "type-specific catalog."
📝 Suggested grammar fixes
-| `catalogDescription` | `string \| CodeRef<ComponentType>` | yes | Description for the type specific catalog. | +| `catalogDescription` | `string \| CodeRef<ComponentType>` | yes | Description for the type-specific catalog. |-| `section` | `string` | yes | Navigation section to which this item belongs to. If not specified, render this item as a top level link. | +| `section` | `string` | yes | Navigation section to which this item belongs to. If not specified, render this item as a top-level link. |frontend/.eslintrc.js (1)
78-83: Consider also restrictingactfromreact-dom/test-utils.The new restriction correctly prevents importing
actfromreact, but the migrated test files were previously usingreact-dom/test-utils. To fully enforce the migration and prevent backsliding, consider adding a similar restriction forreact-dom/test-utils:💡 Optional: Extend restriction to react-dom/test-utils
{ name: 'react', importNames: ['act'], // https://testing-library.com/docs/react-testing-library/api/#act message: "For consistency, import { act } from '@testing-library/react'", }, + { + name: 'react-dom/test-utils', + importNames: ['act'], + message: "For consistency, import { act } from '@testing-library/react'", + },frontend/__mocks__/textEncoderDecoder.ts (1)
5-7: Consider a warning instead of throwing during test setup.The self-documenting guard is appreciated, but throwing an error during test setup could break CI unexpectedly if a future jsdom version adds
TextEncoder/TextDecodersupport. A console warning would be less disruptive while still surfacing the cleanup opportunity.♻️ Alternative approach
if (typeof global.TextEncoder !== 'undefined' || typeof global.TextDecoder !== 'undefined') { - throw new Error('Hello future me. Remove frontend/__mocks__/textEncoderDecoder.ts now please'); + // eslint-disable-next-line no-console + console.warn( + 'TextEncoder/TextDecoder are now natively available. Consider removing frontend/__mocks__/textEncoderDecoder.ts', + ); +} else { + Object.assign(global, { TextDecoder, TextEncoder }); } - -Object.assign(global, { TextDecoder, TextEncoder });
| "@patternfly/react-table": "^6.2.2", | ||
| "@types/node": "^22.19.7", | ||
| "@types/react": "^17.0.39", | ||
| "@types/react": "^18.3.27", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat dynamic-demo-plugin/package.jsonRepository: openshift/console
Length of output: 3730
🌐 Web query:
react-router 5.3.4 React 18 compatibility
💡 Result:
Short answer: react-router v5.3.4 will run with React 18 in many apps, but it is not fully compatible with React 18's new behaviors (particularly StrictMode / concurrent-related semantics) and has reported bugs when used with React 18; upgrading to react-router v6 (or later) is recommended for full React 18 compatibility. [1][2][3]
Sources:
- React Router v5 announcement (background and pre-React‑18 design). [1]
- GitHub issue noting v5 incompatibility with React 18 StrictMode. [2]
- Community reports of problems using react-router v5.3.4 with React 18. [3]
React 18 runtime compatibility issue—react-router v5 not recommended.
While @types/react at 18.3.27 correctly aligns with React 18.3.1 runtime, react-router 5.3.4 has known incompatibilities with React 18, particularly around StrictMode and concurrent semantics. Community reports and upstream guidance recommend upgrading to react-router v6 for full React 18 compatibility.
The mixed setup of react-router 5.3.4 with react-router-dom-v5-compat 6.11.2 requires careful testing; console warnings and potential behavioral issues should be expected. Upgrade to react-router v6 to resolve the incompatibility and align with React 18 best practices.
Also applies to: 33-34
🤖 Prompt for AI Agents
In `@dynamic-demo-plugin/package.json` at line 23, The project currently pins
react-router 5.3.4 and react-router-dom-v5-compat alongside React 18 which is
incompatible; update package.json to replace "react-router" 5.3.4 and
"react-router-dom-v5-compat" with "react-router" (v6) and "react-router-dom"
(v6), then refactor routing usage: replace Switch with Routes, convert Route's
component/render props to element={...}, replace withRouter HOCs with hooks
(useNavigate, useLocation, useParams), and audit any route-related
imports/usages (Switch, Route, withRouter, history-based APIs) to their v6
equivalents to ensure runtime compatibility with React 18.
| "react": "^18.3.1", | ||
| "react-dnd": "^11.1.3", | ||
| "react-dnd-html5-backend": "^11.1.3", | ||
| "react-dom": "^17.0.1", | ||
| "react-dom": "^18.3.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find package.json files to understand workspace structure
fd -t f "package.json" | head -20Repository: openshift/console
Length of output: 1396
🏁 Script executed:
# Check the full frontend/package.json context
head -n 250 frontend/package.json | tail -n 60Repository: openshift/console
Length of output: 2032
🏁 Script executed:
# Look for yarn workspaces or monorepo configuration
grep -r "workspaces\|monorepo" . --include="*.json" 2>/dev/null | head -20Repository: openshift/console
Length of output: 102
🏁 Script executed:
# Check for any lerna or workspace root configuration
cat package.json 2>/dev/null || echo "No root package.json"Repository: openshift/console
Length of output: 81
🏁 Script executed:
# Search for other React dependencies across the repo
rg "\"react\"" --type json -A 1 -B 1Repository: openshift/console
Length of output: 383
🏁 Script executed:
# Get the workspaces configuration
sed -n '/"workspaces"/,/\]/p' frontend/package.json | head -30Repository: openshift/console
Length of output: 139
🏁 Script executed:
# Check React dependencies in key workspace packages
for pkg in frontend/packages/console-app frontend/packages/console-shared frontend/packages/console-plugin-sdk frontend/packages/console-dynamic-plugin-sdk; do
echo "=== $pkg ==="
grep -A 2 '"react"' "$pkg/package.json" 2>/dev/null || echo "No react found"
doneRepository: openshift/console
Length of output: 297
🏁 Script executed:
# Check if workspace packages declare peerDependencies for React
rg "peerDependencies" -A 10 frontend/packages/*/package.json | grep -A 10 "react"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Verify react-dnd@11.1.3 compatibility with React 18
web_searchRepository: openshift/console
Length of output: 111
🏁 Script executed:
# Search for peerDependencies in all workspace packages
find frontend/packages -name "package.json" -exec grep -l "peerDependencies" {} \;Repository: openshift/console
Length of output: 43
🏁 Script executed:
# For packages that have peerDependencies, show them
find frontend/packages -name "package.json" -exec sh -c 'if grep -q "peerDependencies" "$1"; then echo "=== $1 ==="; sed -n '\''/peerDependencies/,/}/p'\'' "$1"; fi' _ {} \;Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check dynamic-demo-plugin dependencies more thoroughly
cat dynamic-demo-plugin/package.json | grep -A 50 "dependencies"Repository: openshift/console
Length of output: 43
🌐 Web query:
react-dnd 11.1.3 React 18 compatibility
💡 Result:
Short answer: No — react-dnd 11.1.3 was released before React 18 and is not officially compatible. The 11.x packages were published years before React 18; react-dnd has continued development and later major releases add modern React support, so you should upgrade to a maintained newer release (or test thoroughly) for React 18. [1][2]
References
- react-dnd-html5-backend v11.1.3 (npm) — published ~5 years ago. [1]
- react-dnd GitHub releases (shows newer major releases after v11). [2]
Singleton React is sound, but react-dnd@11.1.3 isn't compatible with React 18.
Workspace structure correctly hoists React 18.3.1 from root dependencies—workspace packages have no conflicting React declarations. However, react-dnd 11.x was published before React 18 and is no longer officially supported with modern React. Verify this doesn't cause hook stability or rendering issues in your DnD workflows, or upgrade to react-dnd 16+ which has React 18 support.
🤖 Prompt for AI Agents
In `@frontend/package.json` around lines 205 - 208, react-dnd@11.1.3 and
react-dnd-html5-backend@11.1.3 are not officially compatible with React 18 (your
package.json lists "react": "^18.3.1"), so either verify DnD hooks/components
for stability across React 18 or upgrade the DnD libraries: update "react-dnd"
and "react-dnd-html5-backend" to a React-18-compatible major (e.g., react-dnd
16+), run the workspace install/build/tests to ensure hoisting still resolves to
"react": "^18.3.1", and fix any breaking API changes in your DnD usage
(components/hooks referencing react-dnd and react-dnd-html5-backend)
accordingly.
| /** Description for the type specific catalog. */ | ||
| catalogDescription?: string | CodeRef<ReactNode>; | ||
| catalogDescription?: string | CodeRef<ComponentType>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the catalog.ts file
find . -path "*/console-dynamic-plugin-sdk/src/extensions/catalog.ts" -type fRepository: openshift/console
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Search for upgrade-sdk.md in the console-dynamic-plugin-sdk package
find . -path "*/console-dynamic-plugin-sdk/*" -name "*upgrade*" -type fRepository: openshift/console
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Look for all references to catalogDescription in the codebase
rg -n "catalogDescription" --type ts --type tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the actual type definition in catalog.ts
head -50 frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.tsRepository: openshift/console
Length of output: 2296
🏁 Script executed:
#!/bin/bash
# Check upgrade-sdk.md for catalogDescription documentation
cat -n frontend/packages/console-dynamic-plugin-sdk/upgrade-sdk.md | head -100Repository: openshift/console
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Search specifically for catalogDescription in upgrade-sdk.md
grep -n -i "catalogDescription\|catalog.*description" frontend/packages/console-dynamic-plugin-sdk/upgrade-sdk.mdRepository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for references to catalogDescription in the codebase (handle file type error)
rg -n "catalogDescription" frontend/packages/console-dynamic-plugin-sdk/Repository: openshift/console
Length of output: 381
🏁 Script executed:
#!/bin/bash
# Check if there's any migration guidance or breaking changes section in upgrade-sdk.md
grep -n -i "breaking\|migration\|deprecat" frontend/packages/console-dynamic-plugin-sdk/upgrade-sdk.md | head -20Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the full upgrade-sdk.md to see the context around TODO
wc -l frontend/packages/console-dynamic-plugin-sdk/upgrade-sdk.md && cat -n frontend/packages/console-dynamic-plugin-sdk/upgrade-sdk.mdRepository: openshift/console
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Look for any other documentation about this type change in docs
grep -r "CodeRef<ComponentType>" frontend/packages/console-dynamic-plugin-sdk/docs/ 2>/dev/null | head -10Repository: openshift/console
Length of output: 742
🏁 Script executed:
#!/bin/bash
# Check git log to see when this change was made (recent commits)
git log --oneline --all -- frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts | head -10Repository: openshift/console
Length of output: 748
🏁 Script executed:
#!/bin/bash
# Check for any previous version of the type definition (search for ReactNode in catalog context)
rg -B5 -A5 "catalogDescription" frontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdRepository: openshift/console
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Look for any migration notes or breaking changes docs elsewhere
find frontend/packages/console-dynamic-plugin-sdk -name "*.md" -type f | xargs grep -l "breaking\|migration\|4.22"Repository: openshift/console
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Check if there's a CHANGELOG or release notes
find frontend/packages/console-dynamic-plugin-sdk -name "CHANGELOG*" -o -name "RELEASE*" -o -name "HISTORY*"Repository: openshift/console
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Look at the git diff context to see if there's a commit message about this change
git log --all --grep="catalogDescription\|ComponentType" --oneline 2>/dev/null || echo "No matching commits found"Repository: openshift/console
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Get details about the commit that changed ComponentType
git show 80fb815d21 --statRepository: openshift/console
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Get the actual diff for this commit
git show 80fb815d21 -- frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts | head -100Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG-core.md to see if this change is documented
cat -n frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md | head -50Repository: openshift/console
Length of output: 3967
Upgrade documentation for 4.22 breaking changes still incomplete.
The type change for catalogDescription from ReactNode to ComponentType is correctly implemented and documented in CHANGELOG-core.md as a breaking change for console.catalog/item-type ([CONSOLE-4402]). However, the upgrade-sdk.md file for version 4.22 currently contains only "TODO" and lacks the migration instructions referenced in the CHANGELOG. This must be completed before release—plugins consuming catalogDescription as pre-rendered ReactNodes need clear guidance on migrating to ComponentType.
🤖 Prompt for AI Agents
In `@frontend/packages/console-dynamic-plugin-sdk/src/extensions/catalog.ts`
around lines 14 - 15, The upgrade docs are missing migration guidance for the
breaking change where catalogDescription's type changed from ReactNode to
ComponentType (symbol: catalogDescription in console.catalog/item-type); update
upgrade-sdk.md for v4.22 to add clear instructions: explain that pre-rendered
React nodes must be wrapped in a ComponentType (e.g., a functional component
that returns the previous node), show the pattern to accept props if needed, and
provide an example conversion and note on importing ComponentType from React;
reference catalogDescription and the CHANGELOG entry CONSOLE-4402 for context.
| import type { ImpersonateKind } from '@console/dynamic-plugin-sdk'; | ||
| import { useSafetyFirst } from '@console/dynamic-plugin-sdk/src/app/components/safety-first'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate SDK package.json and examine exports
find . -path "*/dynamic-plugin-sdk/package.json" -type f | head -5Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Once we find the SDK, check its exports map and structure
# First, let's find all references to useSafetyFirst in the codebase
rg "useSafetyFirst" --type ts --type tsx -A 2 -B 2Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the actual location of safety-first in the SDK
find . -path "*dynamic-plugin-sdk*" -name "*safety-first*" -type fRepository: openshift/console
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Look for the SDK's index/entry point to see what's publicly exported
find . -path "*/dynamic-plugin-sdk/src/index*" -o -path "*/dynamic-plugin-sdk/dist/index*" | head -10Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find SDK package.json
cat ./frontend/packages/console-dynamic-plugin-sdk/package.json | head -50Repository: openshift/console
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# Search for useSafetyFirst properly (rg doesn't need file type for tsx)
rg "useSafetyFirst" --type ts -A 2 -B 2Repository: openshift/console
Length of output: 15923
🏁 Script executed:
#!/bin/bash
# Check the SDK's src/index files for exports
ls -la ./frontend/packages/console-dynamic-plugin-sdk/src/ | grep -i index
cat ./frontend/packages/console-dynamic-plugin-sdk/src/index.ts 2>/dev/null || cat ./frontend/packages/console-dynamic-plugin-sdk/src/index.js 2>/dev/null || echo "No index file in src"Repository: openshift/console
Length of output: 516
🏁 Script executed:
#!/bin/bash
# Check the safety-first file to see what it exports
cat ./frontend/packages/console-dynamic-plugin-sdk/src/app/components/safety-first.tsx | head -30Repository: openshift/console
Length of output: 1300
🏁 Script executed:
#!/bin/bash
# Check the app entry point to see what's exported from ./app
cat ./frontend/packages/console-dynamic-plugin-sdk/src/app/index.ts 2>/dev/null || echo "No app index"Repository: openshift/console
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Check if useSafetyFirst is re-exported anywhere
rg "export.*useSafetyFirst" --type tsRepository: openshift/console
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Look for any comments or patterns about why this hook isn't exported publicly
rg -B 5 -A 5 "useSafetyFirst" ./frontend/packages/console-dynamic-plugin-sdk/src/app/index.ts 2>/dev/null || echo "Not in app index"Repository: openshift/console
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Check the full deprecation context in safety-first.tsx
cat ./frontend/packages/console-dynamic-plugin-sdk/src/app/components/safety-first.tsxRepository: openshift/console
Length of output: 1873
🏁 Script executed:
#!/bin/bash
# Check what's exported from app/components
cat ./frontend/packages/console-dynamic-plugin-sdk/src/app/components/index.tsRepository: openshift/console
Length of output: 696
🏁 Script executed:
#!/bin/bash
# Verify the full export chain: app -> index.ts
cat ./frontend/packages/console-dynamic-plugin-sdk/src/index.ts | grep -i "app"Repository: openshift/console
Length of output: 103
Replace deprecated useSafetyFirst with standard useState.
useSafetyFirst is marked @deprecated with a clear directive to use useState directly (see CONSOLE-5039 and React 18 migration notes in the hook definition). The hook was originally created to suppress a React warning that React 18 removed as misleading; the protection it provided is no longer necessary.
♻️ Refactor to use useState
-import { useSafetyFirst } from '@console/dynamic-plugin-sdk/src/app/components/safety-first';
-
-const [editable, setEditable] = useSafetyFirst(false);
+const [editable, setEditable] = useState(false);🤖 Prompt for AI Agents
In `@frontend/packages/console-shared/src/utils/pod-ring-utils.ts` around lines 7
- 8, The code imports and uses the deprecated useSafetyFirst hook; replace it
with React's built-in useState by removing the useSafetyFirst import and adding
"import { useState } from 'react'"; then update all usages of useSafetyFirst
(e.g., any local variables or calls named useSafetyFirst) to useState with
identical initial values and consumers so behavior remains the same; ensure the
ImpersonateKind type import stays unchanged and run affected tests/lint to catch
any typing adjustments.
| @@ -1,4 +1,4 @@ | |||
| import { act } from 'react-dom/test-utils'; | |||
| import { act } from '@testing-library/react'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@testing-library/react 16.3.1 act export React 18
💡 Result:
Short answer: Use React's act (not Testing Library's) and ensure versions match — import { act } from 'react'; if you see warnings or missing export on React 18.3.x, upgrade React to a release that exports act (or use unstable_act as a temporary workaround) and upgrade @testing-library/react to a version compatible with your React. [1][2][3]
Quick actionable steps:
- Replace imports: import { act } from 'react' instead of from '@testing-library/react'. [1][2]
- If you get “act does not exist” on React 18.3.x, update React to a patch that provides act (or use import { unstable_act as act } from 'react' temporarily) because some 18.3 releases had that bug. [3]
- Make sure global.IS_REACT_ACT_ENVIRONMENT = true in your test setup if your environment requires it (Jest/jest-environment-jsdom setups usually already handle this). [1]
Sources:
- React act docs. [1]
- testing-library discussion about act deprecation / import. [2]
- React GitHub issue about 18.3 act/unstable_act discrepancy. [3]
🌐 Web query:
react-dom test-utils act vs @testing-library/react act React 18 recommended
💡 Result:
Short answer
- Prefer the act exported from react (or let
@testing-library/reacthandle it). react-dom/test-utils act is deprecated; Testing Library already wraps most helpers in act so you usually don't need to call act yourself. Use the async form (await act(async () => ...)) for async work and ensure IS_REACT_ACT_ENVIRONMENT is set when using act directly. [1][2][3]
Sources
- React docs: act API, async form, and IS_REACT_ACT_ENVIRONMENT note. [1]
- React dev: deprecation guidance for react-dom/test-utils and recommendation to use Testing Library. [2]
- Testing Library docs / issues: RTL helpers auto-wrap in act and guidance to avoid manual act in most cases. [3]
Change act import source to align with React 18 best practices.
The act utility should be imported directly from react, not from @testing-library/react. React 18+ exports act natively, and this is the recommended approach over deprecated react-dom/test-utils. Testing Library's own helpers already wrap operations in act automatically, so importing from react keeps the test setup clean and consistent with official React guidance.
Update to:
import { act } from 'react';🤖 Prompt for AI Agents
In
`@frontend/packages/dev-console/src/components/catalog/providers/__tests__/useDevfileSamples.spec.ts`
at line 1, Update the test to import React's native act by changing the act
import source: replace the current import of the act symbol from
'@testing-library/react' with an import from 'react' (i.e., import { act } from
'react') in the useDevfileSamples.spec.ts test file so the test uses React 18's
built-in act implementation.
| getAppLabels, | ||
| } from '@console/dev-console/src/utils/resource-label-utils'; | ||
| import { useSafetyFirst } from '@console/dynamic-plugin-sdk'; | ||
| import { useSafetyFirst } from '@console/dynamic-plugin-sdk/src/app/components/safety-first'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how useSafetyFirst is imported across the codebase
echo "=== Checking all useSafetyFirst imports ==="
rg -n "import.*useSafetyFirst" --type ts --type tsx
echo ""
echo "=== Check if safety-first module exists and exports properly ==="
fd "safety-first" --type f 2>/dev/null | head -20
echo ""
echo "=== Check the safety-first.tsx file ==="
find . -name "safety-first.tsx" -o -name "safety-first.ts" 2>/dev/null | xargs head -50
echo ""
echo "=== Check public exports from `@console/dynamic-plugin-sdk` ==="
find . -path "*console-dynamic-plugin-sdk*" -name "index.ts" -o -name "index.tsx" 2>/dev/null | xargs ls -la 2>/dev/null | head -20Repository: openshift/console
Length of output: 4654
🏁 Script executed:
#!/bin/bash
# Check if the file mentioned in the review exists
echo "=== Check if knative-plugin file exists ==="
cat frontend/packages/knative-plugin/src/utils/create-channel-utils.ts 2>/dev/null | head -20
echo ""
echo "=== Check SDK public component exports ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/app/components/index.ts 2>/dev/null
echo ""
echo "=== Check if useSafetyFirst is in main SDK exports ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/app/index.ts 2>/dev/null
echo ""
echo "=== Check if useSafetyFirst is in main index.ts ==="
cat frontend/packages/console-dynamic-plugin-sdk/src/index.ts 2>/dev/nullRepository: openshift/console
Length of output: 2335
Remove deprecated useSafetyFirst and use useState directly.
useSafetyFirst is marked @deprecated (see safety-first.tsx) with an explicit TODO to remove it in favor of useState. The React 18 warning it was designed to suppress has been removed as misleading.
Rather than relying on the internal import path, replace this hook with direct useState usage per the deprecation guidance (CONSOLE-5039). This aligns with the removal of the hook from public SDK exports and simplifies the code.
🤖 Prompt for AI Agents
In `@frontend/packages/knative-plugin/src/utils/create-channel-utils.ts` at line
8, Remove the deprecated internal hook import useSafetyFirst and replace calls
with React's useState: delete the import line "useSafetyFirst" from
'@console/dynamic-plugin-sdk/src/app/components/safety-first' and import
useState from 'react' (or use React.useState where appropriate), then update
every usage of useSafetyFirst (e.g., any occurrences like const [foo, setFoo] =
useSafetyFirst(...)) to the equivalent useState initialization and setter (const
[foo, setFoo] = useState(...)). Ensure the semantics (initial value and types)
remain identical and remove any other references to safety-first.tsx per the
deprecation guidance.
|
@logonoff: This pull request references CONSOLE-4402 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 story to target the "4.22.0" version, but no target version was set. This pull request references CONSOLE-4510 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 story to target the "4.22.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. |
|
Review the PR and reply with |
|
Tip For best results, initiate chat on the files or code changes.
Excellent work on this React 18 phase 1 upgrade! After reviewing the changes, I'm impressed with the methodical approach taken here. Let me highlight what stands out: ✅ StrengthsPhased Approach: Deferring Type Safety Improvements: The fix to Testing Modernization: Migrating from Documentation: CHANGELOG entries clearly document breaking changes (
|
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators 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. |
|
Review the PR for changes made to the console API and add the GitHub label |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
I've reviewed the console API changes in this PR. Here's my assessment: 🔍 API Changes SummaryBreaking Changes (Documented in CHANGELOG)
Non-Breaking Additions
✅ AssessmentAll API changes are architecturally sound:
The API changes maintain OpenShift Console's architectural principles while enabling React 18 benefits. The phased approach (deferring I'll add the 🏁 Script executed: Length of output: 252
I've completed the API review, but I'm unable to add the ✅ API Approval ConfirmedHowever, I can confirm that the console API changes are sound and ready for approval:
Recommendation: The The follow-up work you mentioned (SDK migration documentation, etc.) can proceed in parallel with merging this foundational change. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
I've reviewed the console API changes in this PR. Here's my assessment: 🔍 API Changes Analysis✅ Sound Changes
|
|
/label plugin-api-approved |
openshift-ci
bot
added
the
plugin-api-approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coderabbitai[bot], logonoff, vojtechszocs 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 |
|
@logonoff: This pull request references CONSOLE-4402 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 story to target the "4.22.0" version, but no target version was set. This pull request references CONSOLE-4510 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 story to target the "4.22.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. |
|
@logonoff: 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. |
|
/pony Rainbow Dash |
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. |
8 participants
Closes #14686
This PR aims to bump our React dependency to 18. We will NOT be using the new
createRootmethod here, this will be done in a follow-up. Why?@types/reactquicker (namely making React.FCC redundant)Changes:
react,react-dom,@types/react,@types/react-domto 18.xallowObjectInHTMLChildren-- see Cannot pass objects to<Trans>with React 18 type definitions i18next/react-i18next#1483@testing-library/reactso most of our unit tests (except Firehose) are using newcreateRoot, fixed unit tests using AICatalogItemType(we were usingcatalogDescriptionas if it wasComponentTypebut it was typedReactNode)ConsoleDataViewhack: many instances ofConsoleDataVieware using typeTableColumnwhich is incompatible withDataViewTh. Introduce a temporaryConsoleDataViewThtype to fix these type errors, which we will address in a follow-up.