MGMT-22746: Add chatbot app#3350
MGMT-22746: Add chatbot app#3350openshift-merge-bot[bot] merged 4 commits intoopenshift-assisted:masterfrom
Conversation
openshift-ci
bot
added
the
approved
📝 WalkthroughWalkthroughThis PR introduces a new federated micro-frontend application for the Assisted Installer Chatbot while refactoring and upgrading dependencies. It removes the inline chatbot from assisted-ui, upgrades PatternFly packages from v6.2.x to v6.4.x with import path corrections, updates TypeScript to 5.2.2 and React types to 18.2.x, and standardizes UI components using ClusterWizardStepHeader. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser/User
participant Entry as ChatbotMessageEntry
participant Auth as chrome.auth
participant Hook as useAsyncChatbot
participant Client as LightspeedClient
participant API as Lightspeed API
participant Placeholder as AsyncMessagePlaceholder
User->>Entry: Load chatbot message
Entry->>Auth: getToken()
Auth-->>Entry: token
Entry->>Hook: useStateManager()
Hook->>Auth: Check authentication
Auth-->>Hook: isAuthenticated + token
alt Loading State
Hook->>Placeholder: Render skeleton
Placeholder-->>User: Show placeholder
else Authenticated
Hook->>Client: Create LightspeedClient
Client->>API: Subscribe to messages
API-->>Client: Message stream
Client->>Hook: Update state
Hook->>Entry: Render messages with MessageEntry
Entry->>API: onApiCall (fetch with token)
API-->>Entry: Response
Entry-->>User: Display message
else Not Authenticated
Hook-->>User: No chatbot
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes This PR involves heterogeneous changes across multiple areas: a new federated micro-frontend application with multiple components and hooks, repetitive but necessary PatternFly import updates across 9 files, dependency version upgrades across 4 package.json files, removal of legacy chatbot integration code, and UI component refactoring. The new assisted-chatbot app introduces logic density with state management and authentication, while the repetitive import changes reduce overall complexity. Configuration and test updates add variety but are straightforward. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
openshift-ci
bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI Agents
In
@apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx:
- Around line 11-21: The onApiCall useCallback captures chrome.auth.getToken()
but has an empty dependency array; update the dependency array of the
useCallback that defines onApiCall to include chrome.auth (or
chrome.auth.getToken) so the callback is recreated if the chrome.auth reference
changes; ensure any other external helpers used inside (e.g., getBaseUrl) are
also included in the dependency array or otherwise stabilized.
In @apps/assisted-chatbot/src/config.ts:
- Around line 15-27: getBaseUrl currently reads and writes localStorage (via
ENV_OVERRIDE_LOCALSTORAGE_KEY) without guarding against Storage exceptions; wrap
calls to localStorage.getItem and localStorage.setItem in try/catch (or create
small helper functions) so parseEnvQueryParam() fallback logic still works if
localStorage throws, return envs[defaultEnv] when storage access fails, and
ensure you reference getBaseUrl, ENV_OVERRIDE_LOCALSTORAGE_KEY,
parseEnvQueryParam, and envs when applying the fix.
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 43-46: The prop type for conversationId in useAsyncChatbot.tsx is
declared as string but LightspeedMessage defines conversationId as string |
undefined; update the conversationId type in the props object to match (string |
undefined or optional) and adjust any usage inside the hook (e.g., checks/guards
before using conversationId) so callers and internal logic handle the undefined
case consistently.
- Around line 65-94: The useEffect inside useIsAuthenticated uses
chrome.auth.getToken() but has an empty dependency array and uses plain
React.useState which can cause stale closure and state updates after unmount;
update useIsAuthenticated to (1) replace React.useState with useStateSafely for
isLoading and isAuthenticated, (2) include the chrome.auth (or chrome) reference
in the useEffect dependency array so the effect re-runs when chrome.auth
changes, and (3) add a cancellation/mounted flag or rely on useStateSafely to
avoid calling setIsAuthenticated/setIsLoading after unmount (ensure the async
IIFE checks the flag or uses the safe setters before updating state).
- Around line 99-153: The memoized manager captures chrome.auth.getToken inside
fetchFunction but the React.useMemo dependency array is empty, risking a stale
chrome.auth reference; update the dependency array of the React.useMemo that
returns config (the manager) to include chrome.auth (or chrome.auth.getToken) so
the LightspeedClient/fetchFunction is recreated when chrome.auth changes, or
alternatively wrap chrome.auth.getToken in a stable ref/handler and include that
ref/handler in the dependency array; locate the useMemo declaration around
manager, the fetchFunction passed into new LightspeedClient, and ensure the
dependency list reflects the chosen stable dependency.
In @apps/assisted-chatbot/src/types.ts:
- Around line 1-2: The code imports IAIClient from the package
@redhat-cloud-services/ai-client-common (see the import of IAIClient in
apps/assisted-chatbot/src/types.ts) but that package is not listed in
apps/assisted-chatbot/package.json; add
"@redhat-cloud-services/ai-client-common" to the dependencies in that
package.json (matching the appropriate version used across the repo), then run
your package manager (npm install or yarn install) to update node_modules so the
import resolves.
In @apps/assisted-chatbot/tsconfig.json:
- Around line 1-4: tsconfig.json extends "@tsconfig/vite-react/tsconfig.json"
but that package is not declared, so add "@tsconfig/vite-react" to the project
deps (preferably devDependencies) in the appropriate package.json (either
apps/assisted-chatbot/package.json if this is app-scoped or the root
package.json if you manage shared tooling centrally), then run your package
manager install; ensure the dependency is added to devDependencies (e.g.,
"@tsconfig/vite-react": "<latest-compatible-version>") so TypeScript can resolve
the extended config.
In @libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx:
- Around line 7-9: The project still imports legacy @patternfly-6 stylesheet
paths in the Chatbot component; update the stylesheet imports in the Chatbot.tsx
component to use the v6 @patternfly package names instead of @patternfly-6
(replace occurrences of "@patternfly-6/react-core/dist/styles/base.css" and
"@patternfly-6/patternfly/patternfly-addons.css" with the equivalent
"@patternfly/react-core" and "@patternfly/patternfly/addons" v6 stylesheet
import paths as used elsewhere, then verify ChatBotHistory.tsx and Chatbot.tsx
compile without style import errors).
🧹 Nitpick comments (4)
apps/assisted-chatbot/src/types.ts (1)
38-41: Consider stronger typing for component props.The use of
anytype forMessageEntryComponentandFooterComponentreduces type safety. Consider defining specific prop types or using a more constrained generic type.🔎 Example with better typing
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - MessageEntryComponent?: React.ComponentType<any>; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - FooterComponent?: React.ComponentType<any>; + MessageEntryComponent?: React.ComponentType<Record<string, unknown>>; + FooterComponent?: React.ComponentType<Record<string, unknown>>;Or define specific prop interfaces if the expected props are known:
export interface MessageEntryProps { // Define expected props } export interface FooterProps { // Define expected props } export type StateManagerConfiguration<S extends IAIClient> = { // ... other fields MessageEntryComponent?: React.ComponentType<MessageEntryProps>; FooterComponent?: React.ComponentType<FooterProps>; // ... other fields };apps/assisted-chatbot/src/config.ts (2)
9-13: Add error handling for URL API usage.The
URLSearchParamsandwindow.location.searchaccess assumes a browser environment. While this is likely fine for a client-side micro-frontend, consider wrapping in a try-catch to handle edge cases gracefully.🔎 Proposed defensive wrapper
const parseEnvQueryParam = (): string | undefined => { + try { const queryParams = new URLSearchParams(window.location.search); const envVal = queryParams.get('env'); return envVal && envs[envVal] ? envVal : undefined; + } catch { + return undefined; + } };
22-26: Consider more robust environment detection.The host-based environment detection uses simple substring matching which could produce false positives (e.g., a hostname like "devops.example.com" would match 'dev').
🔎 Suggested improvement
Consider using more specific hostname patterns:
const defaultEnv = - window.location.host.includes('dev') || window.location.host.includes('foo') + /\b(dev|foo)\b/.test(window.location.host) || window.location.host.startsWith('dev.') || window.location.host.startsWith('foo.') ? 'staging' : 'production';Or maintain an explicit allowlist of staging hostnames for better reliability.
apps/assisted-chatbot/package.json (1)
5-8: Consider updating the minimum Node.js version.Node.js 16 reached end-of-life in September 2023. Consider requiring Node.js 18 or 20 LTS for better security and support.
🔎 Suggested update
"engines": { - "node": ">=16.0.0", + "node": ">=18.0.0", "npm": ">=7.0.0" },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/assisted-chatbot/src/assets/Ask_Red_Hat_OFFICIAL-whitebackground.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.gitignoreapps/assisted-chatbot/deploy/frontend.yamlapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/AppEntry.tsxapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/src/types.tsapps/assisted-chatbot/src/typings.d.tsapps/assisted-chatbot/tsconfig.jsonlibs/chatbot/lib/components/ChatBot/AIAlert.tsxlibs/chatbot/lib/components/ChatBot/BotMessage.tsxlibs/chatbot/lib/components/ChatBot/ChatBotButton.tsxlibs/chatbot/lib/components/ChatBot/ChatBotHistory.tsxlibs/chatbot/lib/components/ChatBot/ChatBotWindow.tsxlibs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsxlibs/chatbot/lib/components/ChatBot/ExternalLink.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/components/ChatBot/PreviewBadge.tsxlibs/chatbot/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- libs/chatbot/package.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
libs/chatbot/lib/components/ChatBot/BotMessage.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2025-12-17T09:08:18.279Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/cim/components/helpers/toAssisted.ts:199-201
Timestamp: 2025-12-17T09:08:18.279Z
Learning: In libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, the platformType field on agentClusterInstall.spec is guaranteed to always contain a valid PlatformType value (when lowercased), making the type assertion safe without additional runtime validation.
Applied to files:
apps/assisted-chatbot/src/types.ts
📚 Learning: 2025-10-19T17:22:52.502Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
Applied to files:
.gitignore
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
🧬 Code graph analysis (2)
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (3)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
MessageEntryProps(19-25)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-27)libs/ui-lib/lib/ocm/services/UISettingsService.ts (1)
fetch(8-11)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-27)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: translation-files
🔇 Additional comments (21)
libs/chatbot/lib/components/ChatBot/AIAlert.tsx (1)
1-1: LGTM! Import migration from deprecated PatternFly-6 namespace.The import path has been correctly updated from the deprecated
@patternfly-6/react-corenamespace to the standard@patternfly/react-corepackage. No functional changes.apps/assisted-chatbot/src/typings.d.ts (1)
1-4: LGTM! Standard TypeScript declaration for SVG imports.This is standard boilerplate that allows importing SVG files as strings throughout the TypeScript codebase. The declaration is correct and follows common patterns.
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
6-7: LGTM! Import migration from deprecated PatternFly-6 namespace.Both import paths have been correctly updated from the deprecated
@patternfly-6/*namespaces to the standard@patternfly/*packages. No functional changes..gitignore (1)
46-46: LGTM! Correctly unignores the new assisted-chatbot app.The addition follows the existing pattern of unignoring specific apps while keeping
apps/*in the ignore list. This is the correct approach for tracking the new assisted-chatbot application in version control.libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
19-19: FeedbackCard.tsx imports are correctly updated to @patternfly packages, but migration is incomplete across the codebase.The imports in this file have been properly migrated from @patternfly-6/* to @patternfly/* (lines 19 and 22). However, verification reveals remaining @patternfly-6/* usage in
apps/assisted-ui/src/components/Chatbot.tsx(lines 5-7 with CSS imports), contradicting the claim that migration is complete and no mixed usage remains. The systematic migration should be completed across all files, including CSS imports, to fully deprecate the v6 namespace.Likely an incorrect or invalid review comment.
libs/chatbot/lib/components/ChatBot/ChatBotButton.tsx (1)
2-3: LGTM! Clean migration to stable PatternFly packages.The import paths have been correctly updated from the deprecated
@patternfly-6namespace to the stable@patternflyv6 packages.libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
21-21: LGTM! Import path correctly updated.The import path has been correctly migrated from the deprecated
@patternfly-6namespace to the stable@patternflyv6 package.libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (1)
10-10: LGTM! Import path correctly updated.The import path has been correctly migrated from the deprecated
@patternfly-6namespace to the stable@patternflyv6 package.libs/chatbot/lib/components/ChatBot/ExternalLink.tsx (1)
2-3: LGTM! Import paths correctly updated.The import paths have been correctly migrated from the deprecated
@patternfly-6namespace to the stable@patternflyv6 packages.libs/chatbot/lib/components/ChatBot/PreviewBadge.tsx (1)
2-3: LGTM! Import migration looks correct.The migration from
@patternfly-6/*scoped packages to the standard@patternfly/*packages is straightforward and maintains the same component usage.libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
5-7: LGTM! Import migration is consistent.The migration from
@patternfly-6/*to@patternfly/*packages is correct and aligns with the broader migration effort across the codebase.package.json (2)
44-45: Workspace package name is correct.The scripts correctly reference
@openshift-assisted/assisted-installer-ui-chatbot, which matches the package name inapps/assisted-chatbot/package.json.
24-24: The tabbable resolution is necessary and correctly specified.The monorepo uses multiple versions of
@patternfly/react-core(6.2.2 and 6.4.0), which depend on differentfocus-trapversions (6.2.2 and 7.6.4). These in turn require differenttabbableversions (^5.1.4, ^5.3.2, and ^6.2.0 respectively). The"tabbable": "^6.2.0"resolution consolidates these dependencies to a single version, preventing multiple versions oftabbablefrom being installed in the monorepo.apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss (1)
1-3: LGTM!The CSS class correctly extends PatternFly's Skeleton component animation behavior using the appropriate v6 custom property.
apps/assisted-chatbot/src/AppEntry.tsx (1)
1-9: LGTM!The minimal entry point correctly serves its purpose for module federation. The null return is appropriate since this module only exposes federated components without rendering a standalone app.
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
4-35: LGTM! Good accessibility implementation.The component provides an excellent skeleton loading experience with proper screenreader text for each element. The strategic use of the slow animation class on the avatar and timestamp adds visual polish.
apps/assisted-chatbot/deploy/frontend.yaml (1)
1-34: Verify fed-mods.json is generated at the configured path during build/deployment.The Frontend CR configuration correctly references the module federation manifest at
/apps/assisted-installer-ui-chatbot/fed-mods.json, which aligns with the route paths defined infec.config.js. However, confirm that the build process usingfec buildgenerates this artifact at the expected location in the final container image.apps/assisted-chatbot/package.json (1)
25-42: Dependencies look appropriate for the chatbot app.The workspace reference to
@openshift-assisted/chatbotand PatternFly 6.x packages are well-aligned with the PR's objectives. The dependency set is comprehensive for a federated micro-frontend.apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (1)
23-28: LGTM!The
openClusterDetailscallback correctly includeschromeHistoryin the dependency array.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (1)
26-37: LGTM!The
AsyncMessageErrorcomponent provides clear error messaging with appropriate PatternFly styling.apps/assisted-chatbot/fec.config.js (1)
5-35: Configuration looks well-structured for the federated chatbot app.The module federation setup correctly exposes
ChatbotMessageEntryanduseAsyncChatbot. The alias for@openshift-assisted/chatbotproperly points to the shared library.Note that
debug: trueandproxyVerbose: trueare enabled, which is appropriate for development but consider whether these should be controlled via environment variables for production builds.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/assisted-ui/src/components/Chatbot.tsx (1)
26-32: Add timeout to token refresh fetch call.The fetch call to the
/tokenendpoint lacks a timeout, which could cause the chatbot to hang indefinitely if the endpoint is unresponsive. This is especially problematic since token refresh happens automatically when needed.🔎 Proposed fix to add timeout
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout + try { const response = await fetch('/token', { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, body: params.toString(), + signal: controller.signal, }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error(`Token refresh failed: ${response.status}`); } const data = (await response.json()) as { access_token: string; expires_in: number }; token = data.access_token; expiration = Date.now() + data.expires_in * 1000; } catch (error) { + clearTimeout(timeoutId); // eslint-disable-next-line console.error('Failed to refresh token:', error); throw error; }
🤖 Fix all issues with AI Agents
In @apps/assisted-ui/package.json:
- Around line 7-13: The project updated PatternFly dependencies (e.g.,
"@patternfly/patternfly", "@patternfly/react-core", "@patternfly/react-table",
"@patternfly/react-icons") to 6.4.0 but migration steps were not applied; run
the official codemods with npx @patternfly/pf-codemods --v6 ./src --fix to apply
automated fixes across the codebase, manually review and update usages of
components referenced in imports like @patternfly/react-core and
@patternfly/react-table for breaking API/prop changes according to the
PatternFly 6 release notes, fix any remaining compile/test failures, and run the
full test suite to validate the changes.
- Line 6: The dependency entry for "@patternfly/chatbot" uses a non-existent
prerelease version; update the version string in package.json for the
"@patternfly/chatbot" dependency to a valid published release (e.g. "6.4.1" or
"6.3.2") and then regenerate the lockfile by running your package manager (npm
install or yarn install) so package-lock.json / yarn.lock is updated before
committing the change.
In @libs/chatbot/package.json:
- Around line 50-51: Add the missing TypeScript React DOM types to the two UI
apps: update the package.json for apps/assisted-ui and
apps/assisted-disconnected-ui to include "@types/react-dom": "18.2.15"
(preferably as a devDependency to mirror the existing "@types/react" usage) so
their react-dom@^18.2.0 installations have matching type definitions; then run
the workspace install/build to verify TypeScript resolves correctly.
- Line 35: The dependency entry for "@patternfly/chatbot" in libs/chatbot
package.json is using a prerelease version; update the version string from
"6.4.0-prerelease.4" to the stable "6.4.1" in package.json (the dependency key
"@patternfly/chatbot"), then regenerate your lockfile by running your package
manager (npm install / yarn install) and commit the updated lockfile and
package.json so the project uses the stable 6.4.1 release.
- Around line 36-40: Package.json bumps to PatternFly 6.4.0 introduce breaking
changes: run the official PatternFly codemods suite and update component usage
accordingly—search for Button usages and adjust to the new aria-disabled
behavior, replace AccordionContent isHidden with isExpanded, audit Accordion
usage to opt-in or handle new expansion animations, and verify other changed
APIs in
@patternfly/react-core/@patternfly/react-styles/@patternfly/react-tokens; run
the codemods, update prop names and logic (e.g., aria-disabled handling and
animation flags), then run unit/integration/visual tests for the chatbot
components to ensure behavior remains correct.
🧹 Nitpick comments (1)
libs/ui-lib/package.json (1)
47-47: Consider using caret ranges for TypeScript type packages.The @types/react and @types/react-dom packages are pinned to exact versions (18.2.37 and 18.2.15). Consider using caret ranges (e.g.,
^18.2.37) for devDependencies to allow patch and minor updates for type definitions, which are typically non-breaking.🔎 Suggested change
- "@types/react": "18.2.37", + "@types/react": "^18.2.37",- "@types/react-dom": "18.2.15", + "@types/react-dom": "^18.2.15",Also applies to: 49-49
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/package.jsonlibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/assisted-chatbot/package.json
- package.json
- apps/assisted-chatbot/fec.config.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: translation-files
- GitHub Check: tests
🔇 Additional comments (5)
apps/assisted-ui/src/components/Chatbot.tsx (1)
5-7: No action needed—the PatternFly migration is already complete and correct.The unscoped
@patternfly/*packages are already installed inpackage.jsonat version 6.4.0. A codebase search confirms no@patternfly-6imports remain. All PatternFly packages are at compatible versions.Likely an incorrect or invalid review comment.
libs/ui-lib/package.json (2)
39-39: LGTM!The addition of @reduxjs/toolkit as a devDependency is correct, and the version (^1.9.1) is consistent with the peerDependency declaration on line 96. This properly replaces the legacy @redhat-cloud-services/types package.
101-102: React 18 peerDependency update is properly implemented across the codebase.All consuming applications (assisted-ui, assisted-disconnected-ui, assisted-chatbot) have been updated to React 18.2.0, and the codebase correctly uses React 18 rendering APIs (ReactDOM.createRoot). No deprecated React 17 APIs (ReactDOM.render, unmountComponentAtNode) are present in the implementation.
apps/assisted-disconnected-ui/package.json (2)
5-11: Consistent dependency updates across packages.The PatternFly and TypeScript type updates are consistent with other packages in this PR, which helps maintain compatibility across the monorepo.
Also applies to: 34-34
5-11: Check component changelogs if using beta features; standard feature release otherwise.PatternFly 6.4.0 is a minor release (non-breaking by design) with accessibility improvements, new features (high-contrast mode, resizable table columns, animation control), and AI updates. Review the per-package changelogs only if your code uses beta features; otherwise, this is a straightforward stable upgrade. Run any provided codemods before testing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @apps/assisted-chatbot/tsconfig.json:
- Around line 21-22: Remove the trailing comma after the "include" entry in
tsconfig.json (the line containing "include": ["src"],) to make the JSON valid;
edit the tsconfig.json so the include array is the last property without a
trailing comma.
In @apps/assisted-ui/package.json:
- Line 36: Update the @types/react dependency version from "18.2.37" to
"18.2.60" in package.json (the "@types/react" entry), then reinstall/update
lockfile (npm install or yarn install) so the new types are resolved and run a
quick type-check/build to ensure no regressions.
🧹 Nitpick comments (1)
apps/assisted-chatbot/tsconfig.json (1)
6-6: Consider modernizing the compilation target.The
targetis set to"es5", which is quite old. Modern browsers and Node.js versions support ES2015 (ES6) or higher, which would enable smaller bundle sizes and better runtime performance.🔎 Suggested modernization
- "target": "es5", + "target": "ES2015",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
apps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/package.jsonlibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/assisted-chatbot/package.json
- libs/ui-lib/package.json
- package.json
- apps/assisted-disconnected-ui/package.json
- libs/chatbot/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- apps/assisted-chatbot/fec.config.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (2)
apps/assisted-chatbot/tsconfig.json (1)
1-22: Note: Previous review comment about@tsconfig/vite-reactis no longer applicable.The previous review flagged a missing
@tsconfig/vite-reactdependency when the configuration extended that package. The current implementation inlines all compiler options directly without usingextends, which resolves that concern. The configuration is now self-contained.apps/assisted-ui/src/components/Chatbot.tsx (1)
5-7: The CSS import paths are correct and match the official PatternFly v6 documentation. Both@patternfly/react-core/dist/styles/base.cssand@patternfly/patternfly/patternfly-addons.cssare the standard recommended paths for this version.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/assisted-ui/src/components/Chatbot.tsx (2)
11-12: Race condition in token refresh logic.The
getOcmTokenfunction has a race condition: if called multiple times concurrently before the first call completes, each invocation will independently attempt to refresh the token. This could result in:
- Multiple unnecessary token refresh requests
- Potential rate limiting or API throttling issues
- Inconsistent token state
Additionally, the module-level
tokenandexpirationvariables create challenges for testing, hot module reloading, and could cause issues in server-side rendering contexts.🔎 Proposed fix using a promise-based lock
let expiration = Date.now(); let token = ''; +let refreshPromise: Promise<string> | null = null; export const getOcmToken = async () => { // if token expires in less than 5s, refresh it if (Date.now() > expiration - 5000) { + // If a refresh is already in progress, wait for it + if (refreshPromise) { + return refreshPromise; + } + if (!refreshToken) { throw new Error('No refresh token available'); } - const params = new URLSearchParams(); - params.append('grant_type', 'refresh_token'); - params.append('refresh_token', refreshToken || ''); - params.append('client_id', 'cloud-services'); - - try { - const response = await fetch('/token', { - method: 'POST', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - }, - body: params.toString(), - }); - - if (!response.ok) { - throw new Error(`Token refresh failed: ${response.status}`); - } - - const data = (await response.json()) as { access_token: string; expires_in: number }; - token = data.access_token; - expiration = Date.now() + data.expires_in * 1000; - } catch (error) { - // eslint-disable-next-line - console.error('Failed to refresh token:', error); - throw error; - } + + refreshPromise = (async () => { + try { + const params = new URLSearchParams(); + params.append('grant_type', 'refresh_token'); + params.append('refresh_token', refreshToken || ''); + params.append('client_id', 'cloud-services'); + + const response = await fetch('/token', { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + body: params.toString(), + }); + + if (!response.ok) { + throw new Error(`Token refresh failed: ${response.status}`); + } + + const data = (await response.json()) as { access_token: string; expires_in: number }; + token = data.access_token; + expiration = Date.now() + data.expires_in * 1000; + return token; + } catch (error) { + // eslint-disable-next-line + console.error('Failed to refresh token:', error); + throw error; + } finally { + refreshPromise = null; + } + })(); + + return refreshPromise; } return token; };Also applies to: 14-48
42-44: Consider removing console.error in production builds.The
console.errorlogs token refresh failures, which could potentially expose sensitive error details in production environments. Consider using a proper logging service that can be configured to exclude sensitive information or conditionally log based on the environment.🔎 Proposed fix using environment-aware logging
} catch (error) { - // eslint-disable-next-line - console.error('Failed to refresh token:', error); + if (import.meta.env.DEV) { + console.error('Failed to refresh token:', error); + } throw error; }
🤖 Fix all issues with AI Agents
In @libs/ui-lib/package.json:
- Around line 101-102: You changed peerDependencies in package.json to require
React 18 only (react/react-dom set to "^18"), which drops React 17 support;
update the CHANGELOG/release notes to document this breaking change and
explicitly state that React 17 is no longer supported, and bump the package
major version (semver major) to signal the breaking change; also mention
migration guidance for consumers still on React 17 and note any required steps
or timeline.
🧹 Nitpick comments (3)
apps/assisted-disconnected-ui/package.json (1)
5-11: Verify PatternFly 6.4.0 compatibility.The PatternFly packages have been upgraded from 6.2.x to 6.4.0. Ensure the application has been tested with this version and that any breaking changes or deprecations documented in the PatternFly 6 upgrade guide have been addressed.
libs/ui-lib/package.json (1)
49-49: Verify @types/react-dom version alignment.The
@types/react-domversion was changed from^18.3.0to18.2.15, which appears to be a downgrade. While this may be intentional to align with@types/react@18.2.37, verify that this doesn't cause type compatibility issues, especially since the caret range was removed.Run the following to check for type errors:
#!/bin/bash # Check for TypeScript compilation errors after the type dependency changes cd libs/ui-lib yarn check_typesapps/assisted-ui/src/components/Chatbot.tsx (1)
69-73: Consider making the username dynamic.The username is currently hard-coded as
'Assisted Installer user'. If user identification is relevant for the chatbot experience or logging, consider passing the actual authenticated user's name or identifier.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/package.jsonlibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- apps/assisted-chatbot/package.json
- apps/assisted-chatbot/src/config.ts
- apps/assisted-chatbot/tsconfig.json
- libs/chatbot/package.json
- apps/assisted-chatbot/fec.config.js
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: circular-deps
- GitHub Check: tests
🔇 Additional comments (3)
.eslintrc.json (1)
24-27: LGTM!Adding dist directory ignores alongside build paths is the correct pattern for the new app scaffolding. The structure mirrors the existing convention and appropriately excludes generated artifacts from linting.
package.json (1)
45-46: Workspace name for chatbot scripts is correct.The workspace name
@openshift-assisted/assisted-installer-ui-chatbotreferenced in the scripts matches the actual package name inapps/assisted-chatbot/package.json. The scripts will execute without issues.apps/assisted-ui/src/components/Chatbot.tsx (1)
5-7: The CSS imports have been correctly updated as part of the PatternFly v6 migration. All three packages (@patternfly/react-core@6.4.0,@patternfly/chatbot@6.4.0-prerelease.4, and@patternfly/patternfly@6.4.0) are properly declared in dependencies, and the import paths are consistent with patterns used elsewhere in the codebase. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @apps/assisted-chatbot/src/config.ts:
- Around line 15-26: getBaseUrl currently calls localStorage.getItem and
localStorage.setItem directly (using ENV_OVERRIDE_LOCALSTORAGE_KEY) which can
throw SecurityError in restricted/private browsing and crash app; update
getBaseUrl to wrap both the getItem and setItem calls in try/catch, fall back to
undefined/no-op on error, and continue using parseEnvQueryParam() and envs
lookup as before so initialization never throws due to storage access.
🧹 Nitpick comments (4)
apps/assisted-chatbot/tsconfig.json (3)
6-6: Consider a more modern compilation target.The
"target": "es5"setting is quite conservative and was primarily needed for IE11 support. Modern browsers support ES2015+ features, and targeting a newer version can result in smaller bundles and better performance. Unless there's a specific requirement for ES5 compatibility, consider updating to"target": "ES2015"or higher.
1-19: Consider extending a base Vite + React TypeScript configuration.While the manual configuration works, extending a base config like
@tsconfig/vite-reactwould provide well-tested defaults tailored for Vite and React applications, potentially simplifying maintenance and ensuring alignment with ecosystem best practices. The past review comments indicate this was previously attempted.If you decide to use a base config, you would need to:
- Add
@tsconfig/vite-reactto devDependencies- Extend it in tsconfig.json
- Override only the settings specific to your needs
Example configuration extending base
{ + "extends": "@tsconfig/vite-react/tsconfig.json", "compilerOptions": { - "jsx": "react", - "sourceMap": true, - "moduleResolution": "node", - "target": "es5", - "allowSyntheticDefaultImports": true, - "noErrorTruncation": true, "strict": true, - "rootDir": ".", - "skipLibCheck": true, - "lib": ["dom", "esnext"], - "module": "esnext", - "esModuleInterop": true, - "resolveJsonModule": true, - "isolatedModules": true + "rootDir": ".", + "noErrorTruncation": true }, "include": ["src"] }
3-3: Consider updating JSX transform to modern syntax.The
"jsx": "react"setting uses the legacy JSX transform, which requiresimport React from 'react'in every file using JSX. Since this project uses React 18.2.0, switch to"jsx": "react-jsx"to enable the modern transform that eliminates this boilerplate requirement.apps/assisted-chatbot/fec.config.js (1)
8-8: Consider makingdebugconditional on environment.The
debugflag is hardcoded totrue, which may not be appropriate for production builds. Consider making it conditional based onNODE_ENVor a similar environment variable.🔎 Proposed conditional debug setting
- debug: true, + debug: process.env.NODE_ENV === 'development',
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-ui/package.json
- .eslintrc.json
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-chatbot/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- package.json
- libs/chatbot/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: translation-files
🔇 Additional comments (6)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
5-36: Well-structured skeleton placeholder with a minor animation inconsistency to verify.The component implementation follows PatternFly v6 patterns correctly and provides good accessibility with descriptive
screenreaderTextfor each skeleton element.However, I noticed that the
ai-async-message-skeleton--slowclass is applied to the avatar (Line 12) and timestamp (Line 21) skeletons, but not to the message content skeletons (Lines 26, 29, 32). Is this intentional to create a visual distinction between the user/timestamp area and the message content, or should all skeletons animate consistently?libs/ui-lib/package.json (1)
49-49: Verify the downgrade from @types/react-dom @18.3.0 to 18.2.15.The
@types/react-domdependency has been downgraded from18.3.0to a fixed version18.2.15. This is unusual since the newer version is typically preferred for bug fixes and type improvements. Additionally, the caret (^) was removed, making it a fixed version that won't receive patch updates.Please confirm whether this downgrade is intentional (e.g., due to a compatibility issue) or if it should be updated to the current stable version.
apps/assisted-chatbot/fec.config.js (2)
11-11: Verify that stripping PatternFly styles is intentional.Setting
stripAllPfStyles: trueremoves all PatternFly styles. If the chatbot uses PatternFly components, this could cause styling issues. Ensure this is intentional and that the federated setup handles PatternFly styles appropriately through the host application.
16-16: Verify the format fornodeModulesDirectories.The
nodeModulesDirectoriesis set to a string value. Depending on the build tool's expectations, this might need to be an array or absolute path. Verify that this string format is correct for your build configuration.apps/assisted-disconnected-ui/package.json (2)
5-11: PatternFly 6.4.0 versions verified.All seven @patternfly/* packages (patternfly, react-code-editor, react-core, react-icons, react-styles, react-table, react-tokens) at version 6.4.0 exist on npm and are stable releases. No security vulnerabilities detected.
34-34: @types/react 18.2.37 is correctly aligned with React 18.2.0 runtime—no issues found.The version 18.2.37 exists on npm and properly aligns with the React ^18.2.0 runtime dependency. Major version alignment between React and its type definitions is appropriate and correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @apps/assisted-disconnected-ui/package.json:
- Around line 5-11: The dependency bump to PatternFly 6.4.0 for
"@patternfly/patternfly", "@patternfly/react-code-editor",
"@patternfly/react-core", "@patternfly/react-icons", "@patternfly/react-styles",
"@patternfly/react-table", and "@patternfly/react-tokens" may introduce breaking
changes; run the official PatternFly codemods across the repo to update
props/classes/tokens, update any local CSS overrides and CSS variable references
to match 6.4.0 names, review component-level breaking changes in the release
notes and update usages of affected components (e.g., any components imported
from "@patternfly/react-core" or "@patternfly/react-table"), and perform full
functional, accessibility, and visual regression testing before merging.
🧹 Nitpick comments (2)
apps/assisted-chatbot/tsconfig.json (1)
3-3: Consider modernizing JSX and target settings.The current configuration uses older defaults that work but are not optimal for modern React and bundler setups:
- Line 3:
"jsx": "react"- Consider"react-jsx"for the automatic JSX transform (React 17+), which eliminates the need to import React in every file.- Line 6:
"target": "es5"- This is quite conservative. Modern browsers support ES2015+ and your bundler can transpile for legacy targets if needed. Consider"es2015"or newer.🔎 Suggested improvements
- "jsx": "react", + "jsx": "react-jsx", "sourceMap": true, "moduleResolution": "node", - "target": "es5", + "target": "es2015",Also applies to: 6-6
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
5-36: Well-implemented placeholder component with good accessibility.The component structure is clean, uses PatternFly components appropriately, and includes proper
screenreaderTextfor accessibility. The skeleton layout with avatar, timestamp, and content lines provides a good loading UX.💡 Optional: Consider memoization
Since this component has no props and always renders the same output, wrapping it with
React.memocould prevent unnecessary re-renders when the parent updates:-export const AsyncMessagePlaceholder = () => ( +export const AsyncMessagePlaceholder = React.memo(() => ( <div className="pf-v6-u-display-flex pf-v6-u-align-items-flex-start pf-v6-u-p-md"> ... </div> -); +));This is a minor optimization and not critical for this use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/assisted-ui/src/components/Chatbot.tsx
- libs/chatbot/package.json
- package.json
- .eslintrc.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-chatbot/fec.config.js
- apps/assisted-chatbot/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.json
🔇 Additional comments (4)
apps/assisted-ui/package.json (2)
6-13: PatternFly dependencies successfully updated.The
@patternfly/chatbotversion issue has been resolved (now using 6.4.1, a valid published version). The unified upgrade to PatternFly 6.4.x across all packages is consistent and appropriate for the new chatbot app integration.
1-64: Package configuration is well-structured.The package.json follows standard conventions for a monorepo workspace app with Vite + React. Dependencies are organized logically, scripts are comprehensive (build, serve, type-check, lint, format), and the
overridessection appropriately pinsattr-acceptfor compatibility.libs/ui-lib/vitest.config.ts (1)
1-1: LGTM! Good practice to merge default exclusions.Merging Vitest's
configDefaults.excludewith the custombuild/**exclusion ensures that standard test exclusions (such asnode_modules,dist, etc.) are applied alongside your custom rules, improving test discovery hygiene.Also applies to: 14-14
apps/assisted-disconnected-ui/package.json (1)
34-34: Good alignment of React types with runtime version.The update aligns the React type definitions with the React 18.2.0 runtime (line 19), which is a good practice to ensure type safety and avoid type mismatches.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
1-19: The package migration is correct, but fix the type annotation for the Select onSelect handler.The migration from
@patternfly-6/react-coreto@patternfly/react-corev6.4.0 is verified as correct. All imported components are available and compatible with the new package.However, at line 94, the type annotation should match PatternFly's Select onSelect signature. The
valueparameter is optional in PatternFly:Type signature correction
// Current (incorrect): onSelect={(_, val: string) => { // Should be: onSelect={(_, val?: string) => {The defensive coding
${val || ''}suggests awareness of potential undefined values, but the type annotation should reflect PatternFly's actual signature wherevalueis optional, not required.
🤖 Fix all issues with AI Agents
In @apps/assisted-chatbot/package.json:
- Around line 30-33: Update the dependency entry for
"@redhat-cloud-services/frontend-components" in package.json from "7.0.0" to the
latest patch (use "^7.0.22"), then run your package manager (npm/yarn/pnpm) to
regenerate the lockfile so the lock reflects the new version and run tests/build
to verify nothing breaks.
In @libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx:
- Around line 94-96: Update the onSelect handler in FeedbackCard so the second
parameter uses the correct PatternFly type (string | number | undefined) instead
of string; adjust the parameter annotation on the onSelect callback that calls
setCategory and setCategoryOpen (the handler currently using onSelect={(_, val:
string) => { ... }}) to onSelect={(_, val: string | number | undefined) => { ...
}} so the existing defensive `${val || ''}` remains valid and types check
correctly.
🧹 Nitpick comments (3)
apps/assisted-chatbot/fec.config.js (2)
8-10: Consider making debug flags environment-aware.Both
debugandproxyVerboseare unconditionally set totrue, which may produce excessive logging and expose debugging information in production builds. Consider making these environment-aware, similar to howhotReloadis handled on Line 15.🔎 Suggested environment-based configuration
- debug: true, + debug: process.env.NODE_ENV !== 'production', useProxy: true, - proxyVerbose: true, + proxyVerbose: process.env.NODE_ENV !== 'production',
26-28: Document the hardcoded development port.The route configuration uses a hardcoded port
8003for local development. Consider documenting this port requirement in the README or making it configurable through environment variables to prevent port conflicts.package.json (1)
25-25: Consider upgrading tabbable to version 6.3.0.The
"tabbable": "^6.2.0"resolution is compatible with React 18 (tabbable is a framework-agnostic DOM utility with no React peer dependency). However, version 6.3.0 is now available (released December 2025) and includes additional fixes. Update the resolution to"^6.3.0"to benefit from the latest improvements while maintaining compatibility.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- libs/chatbot/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- apps/assisted-chatbot/src/config.ts
- apps/assisted-disconnected-ui/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- .eslintrc.json
- libs/ui-lib/package.json
- apps/assisted-ui/package.json
- libs/ui-lib/vitest.config.ts
- apps/assisted-chatbot/tsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-chatbot/package.jsonlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: lint
- GitHub Check: translation-files
🔇 Additional comments (7)
libs/ui-lib-tests/cypress/views/common.ts (1)
78-78: LGTM! Improved test stability with data-testid selector.The change to use
data-testid="step-header"instead of relying on element types follows testing best practices and makes tests more resilient to markup changes. The timeout increase to 10s suggests accommodation for rendering time—ensure this reflects acceptable page load performance rather than masking underlying issues.libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx (1)
5-7: LGTM! Proper addition of test identifier.Adding
data-testid="step-header"improves testability without affecting component behavior. This aligns well with the corresponding Cypress test update.apps/assisted-ui/src/components/Chatbot.tsx (1)
5-7: LGTM! CSS import paths correctly updated.The migration from
@patternfly-6/*to@patternfly/*package names aligns with PatternFly v6's standard package layout. The import paths are correct and consistent with the project's dependency updates.libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
22-22: Icon import migration looks correct.The migration from
@patternfly-6/react-iconsto@patternfly/react-iconsis straightforward and consistent with the broader package migration.apps/assisted-chatbot/package.json (1)
1-57: Package structure looks good.The package follows monorepo conventions with appropriate engine requirements, standard scripts, and consistent PatternFly dependency versions (6.4.x). The workspace reference to
@openshift-assisted/chatbotcorrectly enables local development.package.json (1)
45-46: LGTM!The new workspace scripts follow the established naming convention and correctly target the new
@openshift-assisted/assisted-installer-ui-chatbotworkspace package.apps/assisted-chatbot/fec.config.js (1)
17-33: Module federation and alias configuration looks good.The module federation correctly exposes
ChatbotMessageEntryanduseAsyncChatbotfor consumption by other micro-frontends. The resolve alias appropriately maps@openshift-assisted/chatbotto the local libs directory, enabling efficient development workflow.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/config.ts:
- Around line 15-26: getBaseUrl currently reads and writes localStorage directly
(via localStorage.getItem and localStorage.setItem) which can throw
SecurityError and crash the app; wrap both localStorage.getItem and
localStorage.setItem accesses in a try/catch inside getBaseUrl so failures fall
back to using parseEnvQueryParam() or envs[defaultEnv] without throwing, and
ensure that when a queryEnv is valid you attempt to persist it but ignore
storage errors (silently or with a safe console.warn) so app initialization
never fails; reference parseEnvQueryParam, ENV_OVERRIDE_LOCALSTORAGE_KEY, and
envs when applying the fix.
In @apps/assisted-ui/package.json:
- Around line 26-27: The package.json currently includes both
"react-router-dom": "5.3.x" and "react-router-dom-v5-compat": "^6.11.2", causing
duplicate/unsupported pairing; consolidate routing to a single approach by
either removing "react-router-dom-v5-compat" and keep v5 imports (e.g.,
BrowserRouter in App.tsx, Route usage) or upgrade to React Router v6 and remove
"react-router-dom@5.3.x", then import BrowserRouter/Routes from v6 and keep
v5-compat for legacy Route/useNavigate usage (update Chatbot.tsx which currently
imports useNavigate from v5-compat accordingly); ensure package.json, App.tsx
(BrowserRouter, CompatRouter, Route) and Chatbot.tsx (useNavigate) imports and
versions are aligned and remove the unused package.
In @libs/ui-lib/package.json:
- Around line 105-107: Confirm and document the intentional downgrade and
peerDependency migration of react-router-dom-v5-compat: validate whether moving
"react-router-dom-v5-compat" from dependencies to peerDependencies and changing
its version from ^6.21.2 to ^6.11.2 (alongside adding "react-router" and
"react-router-dom" as peerDependencies at 5.3.x and restricting React to ^18)
was deliberate; if not, revert the package.json change (restore
react-router-dom-v5-compat to dependencies with the original version) or update
the version to the correct supported release; if it is intentional, add explicit
release notes and a migration guide explaining that consuming apps must now
install react-router-dom-v5-compat and compatible react-router/react-router-dom
v5.3.x, document the rationale for downgrading (e.g., compatibility/bugfix
reference), and update any README or changelog entries to call out the breaking
change and React version support change.
🧹 Nitpick comments (1)
libs/ui-lib-tests/cypress/views/common.ts (1)
77-79: Improved test selector using data-testid.The change to use
data-testid="step-header"makes the test more robust and less brittle than relying on HTML element types. The timeout increase from 3s to 10s should help reduce test flakiness, though it's worth monitoring to ensure it doesn't mask performance degradation.Consider applying a similar data-testid approach to
verifyIsAtSubStep(line 81) for consistency:♻️ Optional consistency improvement
If the sub-step elements also receive data-testid attributes in the component code, update line 81:
verifyIsAtSubStep: (subStepTitle: string) => { - cy.get('h3', { timeout: 2000 }).should('contain.text', subStepTitle); + cy.contains('[data-testid="sub-step-header"]', subStepTitle, { timeout: 10000 }).should('exist'); },Note: This assumes the component has been updated with the appropriate data-testid.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/assisted-chatbot/package.json
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-disconnected-ui/package.json
- package.json
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- libs/chatbot/package.json
- apps/assisted-chatbot/tsconfig.json
- libs/ui-lib/vitest.config.ts
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.jsonlibs/ui-lib/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: format
🔇 Additional comments (10)
.eslintrc.json (2)
9-9: ✓ ESLint config updated to support modern JavaScript syntax.Updating
ecmaVersionto"latest"is a best practice for projects using modern JavaScript/TypeScript features and aligns well with the new assisted-chatbot app using current React patterns.
26-28: ✓ Ignore patterns expanded to cover dist output directories.Adding
distfolder ignores alongside existingbuildfolder patterns is appropriate for modern monorepo tooling. This prevents ESLint from linting generated artifacts across the workspace.apps/assisted-chatbot/src/config.ts (1)
21-24: Verify the 'foo' hostname check is intentional.The hostname check includes
'foo'which appears to be a placeholder or test value. Please confirm this is intentional for your environment detection logic.apps/assisted-chatbot/fec.config.js (3)
5-16: Configuration looks solid.The app configuration is well-structured with appropriate debug, proxy, and hot reload settings. The
sassPrefixderivation from the module name and the monorepo-awarenodeModulesDirectoriessetup follow best practices for micro-frontend architectures.
17-28: Module federation setup is correct.The exposed modules (
ChatbotMessageEntryanduseAsyncChatbot) are properly configured with correct path resolutions, and the local development route setup follows standard patterns.
29-33: Alias configuration is appropriate.The module alias for
@openshift-assisted/chatbotcorrectly points to the monorepo library path, enabling clean imports across the workspace.apps/assisted-ui/package.json (1)
6-13: PatternFly dependencies updated consistently.All PatternFly packages have been updated to 6.4.x versions in a coordinated manner. The previous concern about the prerelease version has been resolved by using the stable 6.4.1 release for chatbot.
libs/ui-lib/package.json (3)
6-12: PatternFly 6.4.0 upgrades look consistent.All PatternFly packages have been upgraded consistently to 6.4.0. Good job ensuring version alignment across the suite.
38-38: Good addition of @reduxjs/toolkit to devDependencies.This aligns with the existing peerDependency requirement and enables proper development/testing.
46-48: React type definitions correctly aligned with React 18.The updates to
@types/reactand@types/react-domproperly support the React 18 requirement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/assisted-chatbot/fec.config.js (1)
1-3: Add error handling for missing package.json properties.The code assumes
insights.appnameexists in package.json without validation. If this property is missing, the application will crash at startup with an unclear error.♻️ Proposed fix with error handling
const path = require('path'); const { insights } = require('./package.json'); +if (!insights?.appname) { + throw new Error('insights.appname is required in package.json'); +} const moduleName = insights.appname.replace(/-(\w)/g, (_, match) => match.toUpperCase());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/assisted-chatbot/package.json
- package.json
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- apps/assisted-chatbot/tsconfig.json
- libs/ui-lib/vitest.config.ts
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- apps/assisted-disconnected-ui/package.json
- libs/chatbot/package.json
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-chatbot/src/config.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.jsonlibs/ui-lib/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (7)
.eslintrc.json (1)
1-30: Configuration changes look good!Both updates are standard and well-supported:
- Using
"ecmaVersion": "latest"(line 9) is appropriate for ESLint 8.36.0 and allows the parser to automatically support the latest ECMAScript features.- Expanding
ignorePatternsto excludedistdirectories alongside existingbuilddirectories (lines 26–28) is a sensible, defensive pattern that aligns with the PR's addition of new apps and potential changes to build output structure.libs/ui-lib/package.json (2)
38-38: LGTM: @reduxjs/toolkit correctly added to devDependencies.The addition of
@reduxjs/toolkitto devDependencies is appropriate—it enables building and testing the library while the peerDependencies entry (line 95) ensures consuming applications provide it at runtime.
46-48: LGTM: React type definitions correctly aligned with React 18.The updated type definitions (
@types/react@18.2.37and@types/react-dom@18.2.15) are properly aligned with the React 18 peer dependency requirements.libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM! Clean refactoring to standardize step header rendering.The replacement of a plain
Contentheader withClusterWizardStepHeaderimproves consistency across the wizard and provides better test stability through thedata-testidattribute. The change preserves the original functionality while making the UI more maintainable.Also applies to: 18-18
apps/assisted-chatbot/fec.config.js (3)
17-25: Module federation configuration looks good.The exposed modules (
ChatbotMessageEntryanduseAsyncChatbot) are properly configured with absolute paths, which is the correct approach for micro-frontend module federation.
8-10: Verify that hardcoded debug flags are appropriate for all environments.The
debugandproxyVerboseflags are hardcoded totrue, which may expose sensitive information or impact performance in production environments. Ensure this configuration is development-only or consider making these settings environment-dependent.
26-28: Theroutesconfiguration in fec.config.js is for the FEC local development server only and is not used in production deployments. The hardcodedlocalhost:8003is intentional and correct for development environments.The comment explicitly indicates this is a dev-only entry point (
AppEntry.tsxstates "Minimal entry point for fec dev server"), and the production deployment is handled separately via the Kubernetes Frontend CRD defined indeploy/frontend.yaml. The build process does not use or transform this routes configuration for production use.No changes are needed. The localhost route is appropriate as-is for local development.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
25-25: Document the rationale for the tabbable resolution.The
tabbable: ^6.2.0resolution pins a transitive dependency required by focus-trap 7.6.4, which is used by @patternfly/chatbot. This addresses a version conflict across the dependency tree. Add a comment in package.json or document in the PR description explaining this resolution ensures future maintainers understand its purpose.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- libs/ui-lib/vitest.config.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- apps/assisted-chatbot/tsconfig.json
- .eslintrc.json
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-ui/package.json
- apps/assisted-chatbot/package.json
- libs/chatbot/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- apps/assisted-chatbot/src/config.ts
- libs/ui-lib-tests/cypress/views/common.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
🔇 Additional comments (5)
libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx (2)
10-15: LGTM! Clean import addition.The import block correctly includes
ClusterWizardStepHeaderalongside the existing wizard components.
172-172: LGTM! Consistent header component usage.The refactoring to use
ClusterWizardStepHeaderimproves consistency across wizard steps while maintaining the same semantic HTML structure (h2 heading).libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx (1)
4-8: LGTM! Improved testability.Adding
data-testid="step-header"enhances test reliability by providing a stable selector for E2E tests, while maintaining the semantic h2 structure.package.json (1)
45-46: LGTM!The new workspace scripts for starting the assisted-chatbot app are correctly configured and follow the established pattern used by other apps in the monorepo.
apps/assisted-chatbot/fec.config.js (1)
1-34: Verify PatternFly styling configuration withstripAllPfStyles: trueThe module federation configuration is well-structured for federated micro-frontends. The exposed components and alias configuration are correctly set up.
However,
useAsyncChatbot.tsx(one of the exposed modules) imports and usesMessagefrom@patternfly/chatbot. SettingstripAllPfStyles: trueon line 11 will remove all PatternFly styles, which may cause styling issues for the PatternFly Message component used in theAsyncMessageErrorcomponent. Verify this flag won't break the chatbot's error message display.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx:
- Around line 94-96: The onSelect handler in FeedbackCard's Select uses an
incorrect type for the second parameter; change the parameter type from string
to string | number | undefined to match PatternFly v6.4.0 and keep the defensive
assignment (setCategory(`${val || ''}`)) and setCategoryOpen(false) as-is;
update the handler signature (onSelect={(_, val: string | number | undefined) =>
{ ... }}) so TypeScript correctly accepts the optional number case and avoids
type errors in the Select component.
🧹 Nitpick comments (2)
apps/assisted-chatbot/fec.config.js (1)
26-28: Consider documenting the development port assignment.The chatbot app is configured to run on
localhost:8003during development. Consider documenting this port assignment in a README or development guide to help developers avoid port conflicts when running multiple apps simultaneously.apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
5-36: Consider improving accessibility for screen reader users.The component structure is clean and functional. However, there are minor accessibility improvements that would enhance the experience for assistive technology users:
Redundant announcements: Lines 26, 29, and 32 all use identical
screenreaderText="Loading message content", which will cause screen readers to announce the same text three times. Consider differentiating them or omitting screenreaderText for the second and third lines.Missing ARIA live region: The container lacks ARIA attributes to announce the loading state. Consider adding
role="status"andaria-live="polite"to the outer div, or wrap the entire placeholder in a container witharia-label="Loading message".♻️ Suggested accessibility improvements
export const AsyncMessagePlaceholder = () => ( - <div className="pf-v6-u-display-flex pf-v6-u-align-items-flex-start pf-v6-u-p-md"> + <div + className="pf-v6-u-display-flex pf-v6-u-align-items-flex-start pf-v6-u-p-md" + role="status" + aria-live="polite" + aria-label="Loading message" + > <div className="pf-v6-u-mr-md"> <Skeleton shape="circle" width="2.5rem" height="2.5rem" className="ai-async-message-skeleton--slow" screenreaderText="Loading user avatar" /> </div> <div className="pf-v6-u-flex-1"> <div className="pf-v6-u-mb-xs"> <Skeleton width="6rem" height="0.875rem" className="ai-async-message-skeleton--slow" screenreaderText="Loading message timestamp" /> </div> <div className="pf-v6-u-mb-xs"> <Skeleton width="85%" height="1rem" screenreaderText="Loading message content" /> </div> <div className="pf-v6-u-mb-xs"> - <Skeleton width="70%" height="1rem" screenreaderText="Loading message content" /> + <Skeleton width="70%" height="1rem" /> </div> <div> - <Skeleton width="55%" height="1rem" screenreaderText="Loading message content" /> + <Skeleton width="55%" height="1rem" /> </div> </div> </div> );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- .eslintrc.json
- libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
- apps/assisted-chatbot/tsconfig.json
- libs/ui-lib/vitest.config.ts
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-disconnected-ui/package.json
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- apps/assisted-chatbot/package.json
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- apps/assisted-ui/package.json
- libs/chatbot/package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: translation-files
🔇 Additional comments (10)
libs/ui-lib-tests/cypress/views/common.ts (1)
78-78: Previously reviewed and addressed.This change was already reviewed in a previous commit (e82e6f5) and approved as an improvement to test selector stability.
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (2)
19-19: LGTM! Clean import of shared component.The import correctly brings in the ClusterWizardStepHeader component from the common module, enabling consistent header rendering across wizard steps.
89-89: Excellent refactoring for test stability.Replacing the direct Content component with ClusterWizardStepHeader improves test reliability through the standardized data-testid attribute while maintaining the same visual presentation and semantic HTML structure.
package.json (2)
25-25: Document the reasoning for the tabbable resolution override.The resolution has been changed from a PatternFly-specific override to
tabbable: ^6.2.0. While tabbable is a focus management library used by PatternFly components, resolution overrides should be documented to explain why they're necessary (e.g., fixing a specific bug, addressing a dependency conflict, or ensuring compatibility with PatternFly 6.4.0).Please confirm this change is intentional and consider adding a comment in the package.json explaining why this resolution is needed, or document it in the PR description.
45-46: LGTM!The new workspace start scripts follow the existing patterns and naming conventions used for other assisted-installer workspaces.
libs/ui-lib/package.json (1)
6-12: LGTM - dependency updates properly addressed.The PatternFly upgrade to 6.4.0, React 18 migration, and router package reorganization have been properly addressed in previous review cycles (commits dd171b2 and e82e6f5). The dependency structure with
@reduxjs/toolkitin both devDependencies and peerDependencies is correct and follows standard patterns.Also applies to: 38-38, 46-46, 48-48, 100-107
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss (1)
1-3: LGTM!The CSS customization properly uses PatternFly v6 custom properties to slow down the skeleton animation for async message placeholders. The class name is clear and descriptive.
apps/assisted-chatbot/fec.config.js (1)
17-25: All exposed module paths verified and exist. The module federation configuration is correct.apps/assisted-chatbot/src/config.ts (1)
15-26: Critical: localStorage access still lacks error handling.As previously flagged, both
localStorage.getItem(line 16) andlocalStorage.setItem(line 18) can throwSecurityErrorwhen storage is disabled or in private browsing mode, causing the application to crash during initialization. This critical issue remains unresolved.Please refer to the previous review comment for the proposed fix using try-catch blocks.
Likely an incorrect or invalid review comment.
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
1-3: LGTM! Clean imports.The imports are properly structured and align with the PatternFly v6 migration mentioned in the PR summary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/config.ts:
- Around line 15-26: getBaseUrl currently calls localStorage.getItem and
localStorage.setItem directly which can throw SecurityError and crash the app;
wrap both accesses in try-catch blocks inside getBaseUrl (around the
parseEnvQueryParam fallback read and the set when queryEnv is valid) so any
thrown error is caught, fall back to treating storage as unavailable (i.e.,
ignore getItem result and skip setItem) and continue returning the appropriate
env from envs based on parseEnvQueryParam() or the defaultEnv logic; reference
parseEnvQueryParam, ENV_OVERRIDE_LOCALSTORAGE_KEY, and envs when locating where
to add the try-catch and fallback behavior.
🧹 Nitpick comments (2)
apps/assisted-chatbot/tsconfig.json (1)
1-19: Previous critical issue resolved; however, the TypeScript configuration has several suboptimal settings.The previously flagged critical issue about the missing
@tsconfig/vite-reactdependency is no longer relevant, as theextendsfield has been removed from the configuration. The trailing comma issue has also been addressed.However, the current configuration contains outdated or non-standard settings:
target: "es5"(line 6): This is outdated for modern applications. Modern browsers support ES2015+ features, and targeting ES5 increases bundle size and compilation complexity unnecessarily.
jsx: "react"(line 3): This is the legacy JSX transform that requiresimport React from 'react'in every file. Modern React 17+ supports the new JSX transform via"jsx": "react-jsx", which eliminates this requirement.
rootDir: "."(line 10): This is non-standard. TherootDirshould typically be"./src"or omitted entirely, as having it point to the package root can cause TypeScript to include unintended files.♻️ Proposed modernization fixes
{ "compilerOptions": { - "jsx": "react", + "jsx": "react-jsx", "sourceMap": true, "moduleResolution": "node", - "target": "es5", + "target": "es2015", "allowSyntheticDefaultImports": true, "noErrorTruncation": true, "strict": true, - "rootDir": ".", "skipLibCheck": true, "lib": ["dom", "esnext"], "module": "esnext", "esModuleInterop": true, "resolveJsonModule": true, "isolatedModules": true }, "include": ["src"] }apps/assisted-chatbot/src/config.ts (1)
21-24: Clarify or remove the 'foo' hostname check.The hostname check for
'foo'(line 22) appears to be test or placeholder code. This could cause unexpected behavior in production if any legitimate hostname contains the substring 'foo'.Either remove this check or add a comment explaining its purpose.
♻️ Proposed fix if 'foo' is test code
const defaultEnv = - window.location.host.includes('dev') || window.location.host.includes('foo') + window.location.host.includes('dev') ? 'staging' : 'production';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.eslintrc.jsonapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/assisted-chatbot/package.json
- libs/ui-lib/vitest.config.ts
- apps/assisted-ui/src/components/Chatbot.tsx
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx
- package.json
- libs/chatbot/package.json
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-ui/package.json
- libs/ui-lib/package.json
- apps/assisted-disconnected-ui/package.json
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: translation-files
🔇 Additional comments (8)
.eslintrc.json (2)
9-9: Good modernization of ECMAScript parsing.Setting
"ecmaVersion": "latest"allows ESLint to understand modern JavaScript features and aligns with current best practices. This is well-supported in ESLint 8.36.0.
26-28: Good expansion of ignorePatterns for dist directories.Adding
"apps/*/dist","libs/*/dist", and"tools/*/dist"to ignorePatterns ensures that generated build artifacts from modern build tools are excluded from linting, keeping the ESLint scope focused on source code. This is consistent with the existingbuilddirectory patterns.libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (1)
19-19: LGTM! Clean refactoring to standardize step headers.The introduction of
ClusterWizardStepHeaderimproves consistency across wizard pages and aligns with the updated test selectors that rely ondata-testid="step-header". The text content and surrounding structure are preserved correctly.Also applies to: 89-89
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM! Consistent header standardization.This follows the same refactoring pattern as
StaticIpPage, replacing the header withClusterWizardStepHeaderto maintain consistency across the cluster wizard. The implementation is correct and preserves the original text and structure.Also applies to: 18-18
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss (1)
1-3: The custom property name is correct and properly implemented.The
--pf-v6-c-skeleton--after--AnimationDurationcustom property is the documented PatternFly v6 API for controlling skeleton animation duration. The 3.6s value is a reasonable override of the default 3s for slow async operations in a chatbot context.apps/assisted-chatbot/fec.config.js (3)
1-16: LGTM: Configuration structure is well-organized.The configuration correctly:
- Derives the module name from
package.jsonwith proper camelCase conversion- Sets up debugging and proxy options for development
- Configures Sass prefix for style isolation
- Handles hot reload via environment variable
The overall structure follows module federation best practices.
17-25: No action needed—exposed files exist.Both exposed files are present in the repository:
ChatbotMessageEntry.tsxexists atapps/assisted-chatbot/src/components/ChatbotMessageEntry/useAsyncChatbot.tsxexists atapps/assisted-chatbot/src/hooks/The module federation configuration is valid.
Likely an incorrect or invalid review comment.
29-33: The alias is correctly configured. Thelibdirectory inlibs/chatbotcontains the source TypeScript files (index.ts,typings.d.ts), not build artifacts. The package's build script compiles TypeScript from thelibdirectory and outputs to abuild/directory separately. The webpack alias pointing to../../libs/chatbot/libis the appropriate source path.
rawagner
force-pushed
the
chatbot_app
branch
2 times, most recently
from
43f1673 to
cd7c5b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 39-63: LSCMessageEntry has two issues: conversationId is typed as
string while LightspeedMessage defines conversationId as string | undefined, and
messageProps sets fallback: null which is immediately overridden by the JSX
fallback prop. Fix by updating the conversationId prop type in the
LSCMessageEntry parameter to match LightspeedMessage (use string | undefined)
and remove the redundant fallback: null from the messageProps object so the
ScalprumComponent uses the JSX fallback={<AsyncMessagePlaceholder />}.
- Around line 65-98: The effect in useIsAuthenticated calls setIsAuthenticated
and setIsLoading after an async fetch without cleanup, risking state updates
after unmount; modify the useEffect in useIsAuthenticated to create an
AbortController (or a mounted flag), pass controller.signal to fetch, and on
cleanup call controller.abort() (or set mounted = false), then guard calls to
setIsAuthenticated and setIsLoading (e.g., if (!signal.aborted) or if (mounted)
) so state updates are only performed when the component is still mounted.
In @libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx:
- Around line 4-8: The ClusterWizardStepHeader component is declared as React.FC
without children typed; change its declaration to include children by using
React.FC<React.PropsWithChildren<{}>> (matching the pattern used in
ClusterWizardStep) so the children prop is explicitly typed for
TypeScript/@types/react compatibility; update the component signature
(ClusterWizardStepHeader) accordingly and ensure React is imported if not
already.
🧹 Nitpick comments (3)
apps/assisted-chatbot/package.json (1)
33-33: Consider pinning a narrower axios version range.The version range
>=0.22.0 <2.0.0spans major versions with potentially breaking changes. Consider narrowing to a specific major version (e.g.,^1.6.0) to avoid unexpected behavior differences across environments.apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (1)
11-23: Potential stale closure overchrome.auth.The
onApiCallcallback has an empty dependency array but captureschrome.auth. Ifchromeis updated during the component's lifecycle, this callback will use a stale reference. WhilegetToken()likely handles token refresh internally, consider either:
- Adding
chrome.authto the dependency array, or- Adding a comment explaining why the stale reference is safe
♻️ Suggested fix if chrome should be in dependencies
const onApiCall = React.useCallback<MessageEntryProps['onApiCall']>(async (input, init) => { const userToken = await chrome.auth.getToken(); const api = new URL(getBaseUrl()); // eslint-disable-next-line @typescript-eslint/restrict-template-expressions return fetch(`https://assisted-chat.${api.hostname}${input}`, { ...(init || {}), headers: { ...(init?.headers || {}), ...(userToken ? { Authorization: `Bearer ${userToken}` } : {}), }, }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [chrome.auth]);apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (1)
100-159: Stalechromereference captured inuseMemo.The
useMemocallback captureschrome(used infetchFunctionat line 108) but has an empty dependency array. This creates a stale closure similar to the one inChatbotMessageEntry.tsx. Ifchrome.authchanges, thefetchFunctionwill use the outdated reference.Additionally, the
handleNewChatfunction (lines 132-136) isasyncbut assigned to a type that may not expect a Promise. While the eslint-disable handles this, ensure the consumer properly handles the returned Promise.♻️ Consider adding chrome.auth to dependencies
const manager = React.useMemo(() => { // ... config creation return config; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [chrome.auth]);Note: This will recreate the client and state manager when auth changes. If that's undesirable, consider extracting the
fetchFunctionto a ref that always has the latestchrome.auth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- libs/locales/tsconfig.json
- libs/ui-lib/tsconfig.json
- apps/assisted-ui/package.json
- package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/chatbot/package.json
- apps/assisted-disconnected-ui/package.json
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
- apps/assisted-ui/src/components/Chatbot.tsx
- libs/ui-lib/vitest.config.ts
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- libs/ui-lib-tests/cypress/views/common.ts
- apps/assisted-chatbot/fec.config.js
- apps/assisted-chatbot/tsconfig.json
- libs/types/tsconfig.json
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- .eslintrc.json
- libs/ui-lib/package.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
🧬 Code graph analysis (2)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (3)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
MessageEntryProps(19-25)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)libs/ui-lib/lib/ocm/services/UISettingsService.ts (1)
fetch(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
🔇 Additional comments (7)
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (2)
7-7: LGTM!The import of
ClusterWizardStepHeaderfrom the common module is correctly structured and aligns with the relative path from this file's location.
17-22: LGTM!Good refactor to use the shared
ClusterWizardStepHeadercomponent. This standardizes header rendering across wizard pages and enables consistent test selectors viadata-testid="step-header".apps/assisted-chatbot/.eslintrc.cjs (1)
1-39: LGTM!The ESLint configuration is well-structured with appropriate overrides for the Vite config and source files. The
no-restricted-importsrule correctly enforces consistentuseTranslationimports from the shared library.apps/assisted-chatbot/src/config.ts (1)
33-36: Verify the hostname substring check for 'foo'.The check for
'foo'in the hostname seems unusual. If this is an internal development environment convention, consider adding a brief comment explaining it for maintainability.apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (1)
25-32: LGTM!The
openClusterDetailscallback correctly includeschromeHistoryin the dependency array and properly navigates to the cluster details page.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
26-37: LGTM!Simple and appropriate error fallback component for the federated message entry.
161-169: LGTM!The conditional return logic correctly handles loading, unauthenticated, and authenticated states, providing appropriate values for each case.
libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 39-63: LSCMessageEntry currently types its conversationId prop as
string but builds a LightspeedMessage where conversationId is defined as string
| undefined; update LSCMessageEntry to accept conversationId?: string (make it
optional) or otherwise match the LightspeedMessage type so it can be undefined,
and ensure any usage of conversationId inside the component handles the
undefined case before passing into the LightspeedMessage payload.
- Around line 65-98: The useIsAuthenticated hook is vulnerable to state updates
after unmount and lacks fetch cancellation; replace React.useState with the safe
hook (useStateSafely) for isLoading and isAuthenticated, and add an
AbortController to the fetch in the effect (pass signal to fetch and call
controller.abort() in the cleanup) so chrome.auth.getToken()/fetch to
https://assisted-chat.${api.hostname}/v1/conversations is cancelled on unmount;
use the safe state setters (from useStateSafely) or check the mounted flag
before calling setIsAuthenticated/setIsLoading to avoid memory leaks.
🧹 Nitpick comments (8)
.eslintrc.json (1)
22-29: Ignore patterns: confirm*/distmatches actual output dirs (Nx/Vite often differ).
Ignoring dist artifacts is good, but depending on the build setup you may be emitting todist/**(e.g.,dist/apps/...) rather thanapps/*/dist. If so, this won’t prevent lint from scanning generated output.Optional hardening (if outputs vary)
"ignorePatterns": [ "apps/*/build", "libs/*/build", "tools/*/build", "apps/*/dist", "libs/*/dist", "tools/*/dist" + ,"dist/**" ]libs/chatbot/package.json (1)
50-51: Consider using caret range for@types/react.The
@types/reactversion is pinned to18.2.37while@types/react-domuses a caret range^18.2.0. This inconsistency may lead to type mismatches if@types/react-domresolves to a different minor version. Consider either pinning both or using caret ranges for both to maintain consistency.Suggested consistency fix
- "@types/react": "18.2.37", + "@types/react": "^18.2.37",apps/assisted-chatbot/package.json (2)
33-33: Narrow the axios version range.The range
>=0.22.0 <2.0.0spans two major versions (0.x and 1.x) with breaking API changes between them. This could cause unexpected runtime behavior depending on how the package manager resolves the dependency.Suggested fix
- "axios": ">=0.22.0 <2.0.0", + "axios": "^1.6.0",Or if you need to stay on 0.x:
- "axios": ">=0.22.0 <2.0.0", + "axios": "^0.28.0",
47-48: Same type definition inconsistency aslibs/chatbot.
@types/reactis pinned to18.2.37while@types/react-domuses^18.2.0. Consider aligning the versioning strategy for consistency across the monorepo.apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (2)
11-23: Suppressed exhaustive-deps warning may hide stale closure issues.The
onApiCallcallback useschrome.auth.getToken()but has an empty dependency array with the exhaustive-deps rule disabled. If thechromereference changes (e.g., on re-mount or context update), this callback will use a stale reference.If
chrome.authis guaranteed to be stable (singleton/memoized by the provider), consider documenting this assumption rather than suppressing the lint rule. Otherwise, includechrome.authin the dependency array.Suggested fix if chrome.auth is stable
const onApiCall = React.useCallback<MessageEntryProps['onApiCall']>(async (input, init) => { const userToken = await chrome.auth.getToken(); const api = new URL(getBaseUrl()); // eslint-disable-next-line @typescript-eslint/restrict-template-expressions return fetch(`https://assisted-chat.${api.hostname}${input}`, { ...(init || {}), headers: { ...(init?.headers || {}), ...(userToken ? { Authorization: `Bearer ${userToken}` } : {}), }, }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [chrome.auth]); // chrome.auth is stable per useChrome() contract
15-15: Consider normalizing the URL input parameter for defensive programming.The URL is constructed as
https://assisted-chat.${api.hostname}${input}. While current usage consistently passes relative paths like/v1/feedback, the type signatureRequestInfo | URLallows broader inputs. Adding explicit normalization would be more defensive:// Example: Ensure input always starts with / const normalizedInput = typeof input === 'string' && !input.startsWith('/') ? `/${input}` : input; return fetch(`https://assisted-chat.${api.hostname}${normalizedInput}`, {This isn't critical given current usage patterns, but improves robustness against future refactoring.
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
100-159: Missing dependency inuseMemomay cause stale closure.The
useMemocallback referenceschrome.auth.getTokenin thefetchFunction, butchromeis not included in the dependency array. While the eslint-disable comment suppresses the warning, this could lead to stale token retrieval if the chrome context changes. Consider addingchrometo the dependency array or extracting the token-fetching logic.Proposed fix
return config; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [chrome]);
131-136: Async handler passed to config without proper error handling.The
handleNewChatis an async function, but errors fromcreateNewConversation()are not caught. If the conversation creation fails, the error will be unhandled andtoggleDrawer(false)will still execute, potentially leaving the UI in an inconsistent state.Proposed fix with error handling
// eslint-disable-next-line @typescript-eslint/no-misused-promises handleNewChat: async (toggleDrawer) => { // can't use hooks here, we are not yet within the correct React context - await stateManager.createNewConversation(); - toggleDrawer(false); + try { + await stateManager.createNewConversation(); + toggleDrawer(false); + } catch (error) { + console.error('Failed to create new conversation:', error); + } },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- libs/types/tsconfig.json
- libs/ui-lib/tsconfig.json
- apps/assisted-chatbot/tsconfig.json
- libs/locales/tsconfig.json
- apps/assisted-chatbot/.eslintrc.cjs
- package.json
- apps/assisted-disconnected-ui/package.json
- apps/assisted-chatbot/src/config.ts
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- apps/assisted-ui/package.json
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
libs/ui-lib/package.jsonlibs/chatbot/package.json
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
📚 Learning: 2025-12-17T09:08:18.279Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/cim/components/helpers/toAssisted.ts:199-201
Timestamp: 2025-12-17T09:08:18.279Z
Learning: In libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, the platformType field on agentClusterInstall.spec is guaranteed to always contain a valid PlatformType value (when lowercased), making the type assertion safe without additional runtime validation.
Applied to files:
libs/chatbot/package.json
🧬 Code graph analysis (2)
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (2)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
MessageEntryProps(19-25)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
🔇 Additional comments (13)
libs/ui-lib-tests/cypress/views/common.ts (1)
77-79: LGTM! More robust test selector using data-testid.The switch from a generic
h2selector to[data-testid="step-header"]makes the test less brittle and aligns with the updatedClusterWizardStepHeadercomponent.Note: The timeout was increased from 3s to 10s. If this is intentional due to known slow loading scenarios, it's fine. Otherwise, consider whether 10s is overly generous and could mask performance regressions.
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM! Consistent header component usage.The switch to
ClusterWizardStepHeaderprovides consistency across wizard steps and enables the standardized data-testid for testing. The nesting structure with the wrapperContentis preserved correctly.Also applies to: 17-19
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (1)
19-19: LGTM! Consistent header component usage.The replacement with
ClusterWizardStepHeaderaligns with the other wizard pages in this PR and enables consistent test targeting via the data-testid attribute.Also applies to: 88-90
libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx (1)
4-8: Remove this suggestion — the codebase uses React 17, not React 18.The component follows the established pattern in the codebase. In React 17 (which is what
@types/react 17.0.xindicates),React.FCincludeschildrenin its props by default. The change to excludechildrenfromReact.FCtyping occurred in React 18. Since this project targets React 17, explicit typing withReact.PropsWithChildrenis unnecessary and inconsistent with other similar components in the codebase (e.g.,ModalDialogsContextProvider,AssistedUILibVersion).Likely an incorrect or invalid review comment.
libs/ui-lib/vitest.config.ts (1)
1-1: Looks good—please verify Vitest supportsconfigDefaultsand thebuild/**glob matches your run root.This is a nice improvement to keep Vitest defaults while excluding build artifacts, but
configDefaultsis version-dependent and'build/**'may not match if Vitest runs from the repo root (you might need'**/build/**'in that case).Also applies to: 14-14
apps/assisted-ui/src/components/Chatbot.tsx (1)
5-7: LGTM!The CSS import paths are correctly updated from the scoped
@patternfly-6/...packages to the standard@patternfly/...naming convention, aligning with the PatternFly 6.4.x dependencies declared in the package manifests.libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (2)
6-7: LGTM!Import paths correctly updated to use the standard
@patternfly/react-coreand@patternfly/react-iconspackages.
23-23: Good type refinement.The explicit function signature
(input: RequestInfo | URL, init?: RequestInit) => Promise<Response>is clearer thantypeof fetchand better documents the expected contract for custom implementations like the one inChatbotMessageEntry.libs/chatbot/package.json (1)
35-40: LGTM!PatternFly dependencies are consistently upgraded to the 6.4.x series across the package.
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (1)
25-30: LGTM!The
openClusterDetailscallback correctly includeschromeHistoryin its dependency array.apps/assisted-chatbot/fec.config.js (1)
5-34: Configuration looks correct for module federation setup.The module federation exposes are correctly configured for
ChatbotMessageEntryanduseAsyncChatbot. The alias resolution for@openshift-assisted/chatbotpoints to the correct workspace path.One minor observation:
debug: trueis hardcoded. If this is intentional for development, consider making it environment-dependent similar tohotReload.libs/ui-lib/package.json (2)
99-104: Peer dependencies correctly updated for React 18 and router compatibility.The
reactandreact-dompeer dependencies are correctly constrained to^18. The addition ofreact-router-dom-v5-compatas a peer dependency is the correct approach for incremental migration from React Router v5 to v6 APIs. Based on learnings, having bothreact-router-domv5.x andreact-router-dom-v5-compatinstalled together is the intended configuration.
6-12: PatternFly upgrade to 6.4.0 is consistent and safe.All PatternFly packages are aligned to version 6.4.0. As a minor version release within the 6.x series, 6.4.0 does not include breaking changes per PatternFly's release policy.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 39-63: LSCMessageEntry declares conversationId as a plain string
but messageProps (LightspeedMessage) can accept undefined; update the component
signature to accept an optional conversationId (change the prop type to
conversationId?: string) and make the messageProps assignment explicitly
defensive (e.g., conversationId: conversationId ?? undefined) so the types align
and no undefined is passed unsafely; adjust any callers if they relied on a
required prop.
- Around line 65-98: The effect inside useIsAuthenticated uses chrome (from
useChrome) and performs an async fetch without cleanup, so add chrome to the
effect dependency array and guard against state updates after unmount; modify
the useEffect in useIsAuthenticated to include chrome (and any other external
values like getBaseUrl if non-stable) in deps and implement an AbortController
(or an isMounted flag / useStateSafely) to cancel the fetch and skip calling
setIsAuthenticated and setIsLoading when aborted/unmounted to prevent the race
condition.
In @package.json:
- Around line 23-25: The root package.json currently forces "@types/react" to
17.0.x via the "resolutions" block, which conflicts with workspace packages that
require 18.2.37; update the "resolutions" entry for "@types/react" to "18.2.x"
in package.json so the workspace resolves to the React 18 typings (or
alternatively add a clear comment/documentation in package.json explaining why
pinning to 17.0.x is intentional and acceptable).
🧹 Nitpick comments (5)
libs/ui-lib-tests/cypress/views/common.ts (1)
77-78: Consider the timeout increase and alignment withverifyIsAtSubStep.Using
data-testidis a solid improvement for test stability. Two observations:
The timeout increased from 3000ms to 10000ms—this could mask future performance regressions. If this increase is intentional (e.g., slower render under certain conditions), that's fine; otherwise, consider keeping it closer to the original.
verifyIsAtSubStep(line 81) still uses a generich3selector with a 2000ms timeout. For consistency, consider updating it to use adata-testidapproach as well.♻️ Optional: Align verifyIsAtSubStep with the new pattern
verifyIsAtSubStep: (subStepTitle: string) => { - cy.get('h3', { timeout: 2000 }).should('contain.text', subStepTitle); + cy.contains('[data-testid="sub-step-header"]', subStepTitle, { timeout: 10000 }).should('exist'); },This would require adding
data-testid="sub-step-header"to the corresponding sub-step header component.apps/assisted-chatbot/src/config.ts (1)
15-38: Consider potential stale localStorage override.The current logic persists the
envquery param to localStorage on line 26, but there's no mechanism to clear the override. If a user visits with?env=stagingonce, they'll remain on staging until they manually clear localStorage, even after the query param is removed.Consider adding a way to clear the override (e.g.,
?env=clearor relying only on query param when present without persisting).♻️ Optional: Add clear mechanism
const parseEnvQueryParam = (): string | undefined => { const queryParams = new URLSearchParams(window.location.search); const envVal = queryParams.get('env'); + if (envVal === 'clear') { + try { + localStorage.removeItem(ENV_OVERRIDE_LOCALSTORAGE_KEY); + } catch (e) { + console.warn('failed to clear override item', e); + } + return undefined; + } return envVal && envs[envVal] ? envVal : undefined; };libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
49-70: Consider handling the case wheremessage.answermight be undefined.On line 57,
message.answeris used directly in the feedback payload. While this is likely fine in practice (bot messages should have answers), the type allowsundefined. The same pattern appears in the feedback copy action on line 131 which handles it with|| ''.♻️ Optional: Consistent undefined handling
body: JSON.stringify({ conversation_id: conversationId, user_question: '', user_feedback: req.userFeedback, - llm_response: message.answer, + llm_response: message.answer || '', sentiment: req.sentiment, categories: req.category ? [req.category] : undefined, }),apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (1)
131-136: AsynchandleNewChatpassed where sync function may be expected.The
handleNewChatfunction is async, and there's an eslint-disable comment for@typescript-eslint/no-misused-promises. This suggests a type mismatch where the callback might be expected to returnvoidbut returnsPromise<void>.While this works at runtime, any errors thrown inside
handleNewChatwould result in an unhandled promise rejection since the caller likely isn't awaiting or catching.♻️ Wrap async logic with error handling
- // eslint-disable-next-line @typescript-eslint/no-misused-promises - handleNewChat: async (toggleDrawer) => { - // can't use hooks here, we are not yet within the correct React context - await stateManager.createNewConversation(); - toggleDrawer(false); - }, + handleNewChat: (toggleDrawer) => { + // can't use hooks here, we are not yet within the correct React context + void stateManager.createNewConversation() + .then(() => toggleDrawer(false)) + .catch((error) => { + // eslint-disable-next-line no-console + console.error('Failed to create new conversation:', error); + }); + },libs/ui-lib/package.json (1)
45-47: Minor inconsistency in version specifiers.
@types/reactis pinned exactly (18.2.37) while@types/react-domuses a caret range (^18.2.0). Consider using consistent versioning style. Note: the rootpackage.jsonresolution for@types/react: 17.0.xwill override this declaration regardless.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/index.tslibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- libs/ui-lib/tsconfig.json
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- libs/chatbot/package.json
- apps/assisted-chatbot/package.json
- apps/assisted-chatbot/fec.config.js
- apps/assisted-ui/package.json
- libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
- apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
- libs/types/tsconfig.json
- apps/assisted-chatbot/tsconfig.json
- apps/assisted-ui/src/components/Chatbot.tsx
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- .eslintrc.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
libs/chatbot/lib/components/ChatBot/MessageEntry.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
libs/ui-lib/package.jsonapps/assisted-disconnected-ui/package.json
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
🧬 Code graph analysis (2)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
libs/chatbot/lib/index.ts (3)
ApiCallInit(4-4)ApiCallResponse(4-4)MessageEntryProps(4-4)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: circular-deps
🔇 Additional comments (18)
libs/locales/tsconfig.json (1)
8-8: LGTM!Adding
skipLibCheck: trueis a sensible optimization that aligns with the same configuration change applied to other library tsconfigs in this PR (libs/typesandlibs/ui-lib). This improves build performance by skipping type checking of declaration files.libs/ui-lib/vitest.config.ts (1)
1-1: LGTM!Good practice to extend
configDefaults.excluderather than replacing it entirely. This preserves Vitest's default exclusions (likenode_modules,dist, etc.) while adding the project-specificbuild/**exclusion to avoid running tests on compiled artifacts.Also applies to: 14-14
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (1)
19-19: LGTM!The import and usage of
ClusterWizardStepHeaderaligns with the consistent header pattern being applied across other wizard steps (CustomManifestsPage, Day2StaticIP). This provides uniform styling and includesdata-testidsupport for UI testing.Also applies to: 89-89
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM!The migration to
ClusterWizardStepHeaderaligns with the centralized step header styling pattern used across other wizard pages. The component structure remains clean, and the existingContent/ContentVariantsimports are still utilized appropriately for the wrapper and description text.Also applies to: 17-18
libs/ui-lib-tests/cypress/views/common.ts (1)
104-107: No action needed.The
visitClusterDetailsPagefunction will continue to work correctly. TheClusterWizardStepHeadercomponent renders as an<h2>element via PatternFly's Content component (withcomponent="h2"), so thecy.get('h2').should('exist')assertion remains valid and will not fail.Likely an incorrect or invalid review comment.
apps/assisted-chatbot/src/config.ts (1)
1-5: LGTM! Environment configuration is well-structured.The environment map clearly defines the API endpoints for each environment.
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (2)
6-7: LGTM! PatternFly imports updated correctly.The import paths
@patternfly/react-coreand@patternfly/react-iconsare correct for PatternFly v6.
19-30: LGTM! Well-designed minimal types for API abstraction.The custom
ApiCallInitandApiCallResponsetypes provide a clean abstraction overfetchwithout requiring DOM globals, which is appropriate for library code that may be consumed in various build environments.libs/chatbot/lib/index.ts (1)
4-4: LGTM! Public API surface correctly expanded.The new types
ApiCallInitandApiCallResponseare properly re-exported from the library's main entry point.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
100-169:useMemocreates client/manager once butchromecould change.The
useMemohas an empty dependency array (with eslint-disable), meaningchromeis captured once. Ifchrome.auth.getTokenreturns different tokens over time (e.g., token refresh), the closure infetchFunction(line 108) will still use the currentchromereference, which should be fine since it's callinggetToken()on each request.However, consider whether
isAuthenticatedshould trigger re-creation of the manager. Currently, the manager is created regardless of auth state, and auth filtering happens at the return level (lines 161-166).The current pattern seems intentional - manager is created once, and auth state gates what's returned. Verify this is the desired behavior, especially for scenarios where auth state changes after initial load.
26-37: LGTM! Clear error state component.The
AsyncMessageErrorcomponent provides a user-friendly error message with appropriate styling.package.json (1)
44-45: LGTM!The new chatbot scripts follow the existing naming convention and are consistent with other app scripts in the workspace.
apps/assisted-chatbot/.eslintrc.cjs (1)
1-39: LGTM!Well-structured ESLint configuration:
- Appropriate separation of overrides for build config vs source files
- The
no-restricted-importsrule foruseTranslationenforces consistent i18n usage across the codebasebrowser: falseandno-console: 'off'are sensible for the Vite config filelibs/ui-lib/package.json (2)
6-12: LGTM!Consistent PatternFly 6.4.0 upgrade across all related packages. The coordinated version bump ensures compatibility within the workspace.
99-104: LGTM!React 18 peer dependencies and
react-router-dom-v5-compataddition are appropriate. Based on learnings, the compat package is designed to work alongside react-router-dom v5 for incremental migration.apps/assisted-disconnected-ui/package.json (3)
5-11: LGTM!PatternFly 6.4.0 and React 18 upgrades are consistent with the rest of the workspace. The type definitions align with the runtime dependencies.
Also applies to: 19-20, 34-35
24-25: LGTM!The router configuration is correct. Based on learnings,
react-router-dom-v5-compatis designed to work alongsidereact-router-domv5 for incremental migration to v6 APIs.
4-4: The@openshift-console/dynamic-plugin-sdk1.0.0 upgrade introduces breaking changes; verify they are properly mitigated.The major version bump (0.0.3 → 1.0.0) brings breaking changes including React Router v6 migration. This is correctly handled in the PR via the
react-router-dom-v5-compatdependency already present inpackage.jsonand imported inApp.tsx. Other breaking changes (Console 4.15 requirements, webpack config) do not apply to this standalone disconnected UI. PatternFly alignment is addressed via the 6.4.0 updates. Confirm that any other SDK APIs used inlibs/ui-libare compatible with 1.0.0.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
49-70: Populateuser_questionfield in feedback submission.The
user_questionfield is hardcoded to''whileBotMessage.tsxproperly supplies the user message viauserMsgprop.MessageEntryshould receive the user question as a prop (similar toBotMessage) to maintain consistent feedback context across both components when calling the feedback API.
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/package.json:
- Around line 5-8: The engines node version in apps/assisted-chatbot's
package.json ("engines": { "node": ">=16.0.0" }) conflicts with the monorepo
root package.json baseline (currently Node 14); choose one of two fixes: either
update the monorepo baseline node engine to ">=18.0.0" across the root
package.json and communicate/test the upgrade, or change apps/assisted-chatbot's
"node" engine to match the root (lower to the root value) and schedule a
coordinated repo-wide upgrade; edit the "engines" -> "node" field in the
appropriate package.json (apps/assisted-chatbot/package.json or root
package.json), run CI/tests and update repo docs/README and any CI node versions
to reflect the chosen Node LTS.
In @libs/ui-lib/package.json:
- Around line 99-100: The peerDependencies in libs/ui-lib/package.json currently
require "react" and "react-dom" as "^18", which is a breaking change for
consumers on React 17; either revert the peerDeps to support both versions by
changing them back to "react": "^17 || ^18" and "react-dom": "^17 || ^18", or if
the intention is to drop React 17 support, bump the package major version and
clearly document this breaking change in the release notes and changelog so
consumers are warned; update package.json and release docs accordingly (look for
the peerDependencies entries "react" and "react-dom").
🧹 Nitpick comments (7)
libs/ui-lib/package.json (1)
45-45: Consider using a caret range for@types/react.The version is pinned exactly to
18.2.37without a caret (^). Type definitions typically use ranges to automatically pick up patch fixes. This exact pinning may require manual updates for type fixes.Suggested fix
- "@types/react": "18.2.37", + "@types/react": "^18.2.37",apps/assisted-chatbot/src/config.ts (1)
33-37: Hostname substring match may be overly broad.Using
includes('dev')could unintentionally match production-like domains containing "dev" (e.g.,developer.openshift.com). Consider using more precise matching.Suggested refinement
const defaultEnv = - window.location.host.includes('dev') || window.location.host.includes('foo') + /\b(dev|stage|foo)\b/.test(window.location.host) || window.location.host.startsWith('localhost') ? 'staging' : 'production';libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
115-120: Positive feedback submission silently ignores errors.The positive feedback
onClickusesvoid onFeedbackSubmit(...)without error handling. If the API call fails, users won't receive any indication. Consider adding error handling or user feedback consistent with the negative feedback flow.Example improvement
positive: { ariaLabel: 'Good response', tooltipContent: 'Good response', clickedTooltipContent: 'Feedback sent', onClick: () => { - void onFeedbackSubmit({ + onFeedbackSubmit({ userFeedback: '', sentiment: 1, - }); + }).catch((e) => { + // eslint-disable-next-line no-console + console.error('Failed to submit positive feedback:', e); + }); }, },libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
94-97: Remove overly narrow type annotation on Select callback parameter.PatternFly's Select
onSelectcallback signature acceptsvalue?: SelectOptionProps['value'](which can beundefined), not strictlystring. The explicitstringtype annotation masks this possibility and is inaccurate.Consider using the correct type
- onSelect={(_, val: string) => { - setCategory(`${val || ''}`); + onSelect={(_, val) => { + setCategory(String(val ?? ''));apps/assisted-chatbot/package.json (1)
33-33: Narrow axios version to a single major version for consistency across the codebase.The range
>=0.22.0 <2.0.0allows both 0.x and 1.x versions, which have breaking changes between them (headers shape, FormData handling, parameter serialization). This creates inconsistency:libs/ui-libuses<1.0.0andlibs/sdksuses^1.12.0, while this package allows both. Pick one major version constraint.Suggested update
- "axios": ">=0.22.0 <2.0.0", + "axios": "^1.6.0",apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
43-46: Type inconsistency:conversationIdis non-nullable here but nullable inLightspeedMessage.The prop type declares
conversationId: stringbutLightspeedMessage(lines 17-24) defines it asstring | undefined. Consider aligning the types:}) => { - message: MessageType<LightSpeedCoreAdditionalProperties>; - avatar: string; - conversationId: string; + message: MessageType<LightSpeedCoreAdditionalProperties>; + avatar: string; + conversationId: string | undefined; }) => {
67-68: Potential memory leak: useuseStateSafelyfor async state updates.Based on project patterns,
useStateSafelyshould be used instead ofReact.useStatewhen state is updated after asynchronous operations to prevent state updates on unmounted components.+import { useStateSafely } from '...'; // adjust import path const useIsAuthenticated = () => { const chrome = useChrome(); - const [isLoading, setIsLoading] = React.useState(true); - const [isAuthenticated, setIsAuthenticated] = React.useState(false); + const [isLoading, setIsLoading] = useStateSafely(true); + const [isAuthenticated, setIsAuthenticated] = useStateSafely(false);Based on learnings from this repository.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/package.jsonapps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/index.tslibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/@types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- libs/ui-lib/tsconfig.json
- package.json
- apps/assisted-chatbot/tsconfig.json
- apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
- apps/assisted-ui/package.json
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- apps/assisted-chatbot/.eslintrc.cjs
- apps/assisted-chatbot/fec.config.js
- libs/locales/tsconfig.json
- libs/types/tsconfig.json
- libs/chatbot/package.json
- .eslintrc.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-ui/src/components/Chatbot.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
libs/ui-lib/package.jsonapps/assisted-disconnected-ui/package.json
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
🧬 Code graph analysis (2)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
libs/chatbot/lib/index.ts (3)
ApiCallInit(4-4)ApiCallResponse(4-4)MessageEntryProps(4-4)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
🔇 Additional comments (23)
libs/ui-lib/vitest.config.ts (1)
1-14: LGTM! Good practice to extend rather than replace default excludes.Spreading
configDefaults.excludeensures Vitest's default exclusions (likenode_modules,dist,.git, etc.) are preserved while adding the custombuild/**exclusion. This prevents accidentally running tests on directories that should be excluded by default.libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM!The header standardization using
ClusterWizardStepHeaderaligns with the PR's pattern for consistent wizard step headers and enables reliable test selectors viadata-testid="step-header".Also applies to: 17-22
libs/ui-lib-tests/cypress/views/common.ts (1)
77-79: LGTM!Good improvement using
data-testidfor stable test selectors. The 10-second timeout provides adequate time for wizard step transitions, andcy.containswith the attribute selector ensures both the element identity and text content are verified.libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (1)
19-19: LGTM!Consistent header standardization following the same pattern as other wizard step pages. The
ClusterWizardStepHeadercomponent provides thedata-testid="step-header"attribute for test reliability.Also applies to: 88-93
apps/assisted-ui/src/components/Chatbot.tsx (3)
5-7: LGTM!The CSS import paths correctly reference the standard
@patternflypackages, aligning with the installed PatternFly 6.x dependencies.
9-48: No changes detected in token management logic.
50-75: No changes detected in ChatBot component.libs/ui-lib/package.json (3)
47-47: Verify the@types/react-domversion change.The version appears to have been changed from
^18.3.0to^18.2.0. If this is an intentional downgrade for compatibility reasons, consider adding a comment explaining why. Otherwise,^18.3.0would include newer type definitions.
104-104: Movingreact-router-dom-v5-compatto peerDependencies is appropriate.This correctly delegates the responsibility of providing this package to the consuming application, allowing for proper version management. Based on learnings, this package is designed to work alongside react-router-dom v4/v5 for incremental migration to v6 APIs.
6-12: PatternFly version 6.4.0 does not exist on npm; the latest available version is 6.3.1.Upgrading to a non-existent version will cause dependency resolution failures. Update all PatternFly packages in lines 6-12 to 6.3.1 or verify if 6.4.0 is intended as a pre-release dependency.
⛔ Skipped due to learnings
Learnt from: rawagner Repo: openshift-assisted/assisted-installer-ui PR: 3350 File: apps/assisted-ui/package.json:26-27 Timestamp: 2026-01-07T15:19:02.310Z Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
19-22: LGTM on PatternFly import migration.The import paths have been correctly updated from
@patternfly-6/*to@patternfly/*, aligning with the standard PatternFly v6 package naming convention.apps/assisted-chatbot/package.json (1)
22-40: LGTM on dependency setup.The dependency configuration is well-structured with appropriate workspace references, pinned PatternFly versions for consistency, and reasonable dev tooling. The
insights.appnameconfiguration aligns with the Red Hat Cloud Services frontend pattern.libs/chatbot/lib/index.ts (1)
4-4: LGTM on expanded type exports.The public API surface is cleanly extended to expose
ApiCallInitandApiCallResponsetypes alongsideMessageEntryProps, enabling consumers to properly type theironApiCallimplementations.apps/assisted-chatbot/src/config.ts (1)
15-32: LGTM on environment override logic.Good defensive coding with try/catch around localStorage operations (handles private browsing mode and storage quota exceptions). The query parameter taking precedence over stored values with persistence is a developer-friendly pattern.
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (2)
19-30: LGTM on inline type definitions.Defining
ApiCallInitandApiCallResponseinline avoids dependency on DOM globals that may not exist in all build environments. The types are appropriately minimal while covering the necessary response properties.
6-7: LGTM on PatternFly import migration.Import paths correctly updated from
@patternfly-6/*to the standard@patternfly/*packages.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
161-169: LGTM!The conditional return logic correctly handles loading and authentication states, returning appropriate values that match the
UseManagerHooktype.
70-92: Missing error handling forgetToken()call.If
chrome.auth.getToken()throws (line 73), the error is unhandled andisLoadingwill remaintrueindefinitely since thefinallyblock won't execute.🛠️ Suggested fix
React.useEffect(() => { void (async () => { - const api = new URL(getBaseUrl()); - const token = await chrome.auth.getToken(); try { + const api = new URL(getBaseUrl()); + const token = await chrome.auth.getToken(); const response = await fetch(`https://assisted-chat.${api.hostname}/v1/conversations`, { method: 'GET', headers: token ? { Authorization: `Bearer ${token}`, } : {}, }); setIsAuthenticated(response.ok); } catch (error) { // eslint-disable-next-line no-console console.error('Error checking authentication:', error); } finally { setIsLoading(false); } })();Likely an incorrect or invalid review comment.
103-159: Thechromereference fromuseChrome()is stable and designed to be captured without dependency array updates.useChrome()is from the established@redhat-cloud-services/frontend-componentslibrary (v7.0.0), which returns a stable reference. The pattern of capturingchromein closures with an empty dependency array is intentional and consistently used throughout the codebase (including inuseIsAuthenticatedandChatbotMessageEntry), confirming this is an accepted approach. The eslint-disable comment acknowledges the dependency linter rule and the developer's conscious choice. No action needed.apps/assisted-disconnected-ui/package.json (4)
24-25: Version constraint change for react-router-dom.The version specifier changed from
^5.3.3to5.3.x. This is slightly more restrictive —^5.3.3would allow5.4.0if released, while5.3.xlocks to5.3.*patch releases only. This appears intentional given the presence ofreact-router-dom-v5-compatfor incremental migration to v6. Based on learnings, having both packages installed together is the correct configuration for this migration strategy.
34-35: Type definitions correctly aligned with React 18.The
@types/reactupgrade from 17.0.x to 18.2.37 and addition of@types/react-domat ^18.2.0 properly align with the React 18.2.0 runtime dependency declared on line 19-20. Good consistency.
4-4: Be aware of React version mismatch in SDK dependency.The @openshift-console/dynamic-plugin-sdk v1.0.0 declares a dependency on React ^17.0.1, but the apps use React ^18.2.0. While React 18 is generally backward compatible with libraries targeting React 17, this version mismatch is worth noting. Verify that all usages of the SDK (Patch, WatchK8sResource, K8sResourceCommon, etc.) work as expected with React 18 in production.
5-11: PatternFly packages upgraded to 6.4.0 — no breaking changes expected per PatternFly versioning policy.All PatternFly packages updated to 6.4.0 (a minor release). PatternFly reserves breaking changes for major releases only; 6.4.0 introduces beta features (high-contrast mode, animation controls) and accessibility improvements but no API breaking changes. Still recommend running the PatternFly codemods (class-name-updater, tokens-update, css-vars-updater) and testing to catch any CSS overrides or component usages that may need adjustment.
rawagner
force-pushed
the
chatbot_app
branch
2 times, most recently
from
c81df58 to
a9ef6a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/package.json:
- Line 33: The axios dependency in apps/assisted-chatbot's package.json is too
broad (">=0.22.0 <2.0.0") and must be aligned with the monorepo; change the
"axios" entry to match the monorepo standard used by either libs/ui-lib
("<1.0.0") or libs/sdks ("^1.12.0") depending on compatibility, then regenerate
the lockfile (npm/yarn install) and run the test/build to ensure nothing breaks;
look for the "axios" dependency line in package.json and update it to the chosen
pinned range, and run the repo's install/test steps to finalize.
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 39-63: LSCMessageEntry currently types conversationId as required
string but LightspeedMessage allows it to be undefined; change the component
prop to accept an optional conversationId (e.g., conversationId?: string) in the
LSCMessageEntry parameter typing so the value can be passed through to
messageProps without a type error, leaving the assignment
messageProps.conversationId = conversationId as-is.
🧹 Nitpick comments (5)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
19-36: Custom API types are reasonable but limited.The
ApiCallResponsetype deliberately omits body-reading methods (json(),text()) to avoid DOM globals. This works for the currentonFeedbackSubmitusage which only checks status codes, but consumers needing response bodies will require a different approach.If future use cases need response data, consider extending
ApiCallResponseor documenting this limitation.apps/assisted-chatbot/package.json (1)
5-8: Consider updating minimum Node.js version.Node.js 16 reached end-of-life in September 2023. Consider updating the minimum to Node 18 LTS for security and compatibility.
apps/assisted-chatbot/fec.config.js (1)
8-10: Consider making debug settings environment-dependent.
debug: trueandproxyVerbose: trueare hardcoded. For production builds, consider making these configurable similar tohotReload.♻️ Optional: Environment-based debug settings
- debug: true, + debug: process.env.DEBUG === 'true', useProxy: true, - proxyVerbose: true, + proxyVerbose: process.env.DEBUG === 'true',apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (1)
65-98: Consider memory leak protection for async state updates.The
useIsAuthenticatedhook performs asynchronous operations and updates state after the promise resolves. If the component unmounts before the fetch completes, this can cause memory leaks or React warnings about updating unmounted components.Based on learnings,
useStateSafelycustom hook should be used instead ofReact.useStateto prevent state updates on unmounted components.♻️ Suggested fix using abort controller
const useIsAuthenticated = () => { const chrome = useChrome(); const [isLoading, setIsLoading] = React.useState(true); const [isAuthenticated, setIsAuthenticated] = React.useState(false); React.useEffect(() => { + const abortController = new AbortController(); void (async () => { const api = new URL(getBaseUrl()); const token = await chrome.auth.getToken(); try { const response = await fetch(`https://assisted-chat.${api.hostname}/v1/conversations`, { method: 'GET', headers: token ? { Authorization: `Bearer ${token}`, } : {}, + signal: abortController.signal, }); setIsAuthenticated(response.ok); } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + return; + } // eslint-disable-next-line no-console console.error('Error checking authentication:', error); } finally { - setIsLoading(false); + if (!abortController.signal.aborted) { + setIsLoading(false); + } } })(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + return () => abortController.abort(); + }, [chrome.auth]); return { isAuthenticated, isLoading, }; };apps/assisted-disconnected-ui/package.json (1)
34-35: React 18 types alignment is correct.Updating the type definitions to match React 18 is appropriate since
reactandreact-domare already at^18.2.0.Minor inconsistency:
@types/reactis pinned (18.2.37) while@types/react-domuses a caret (^18.2.0). Consider aligning the version specifier style for consistency.Suggestion for consistency
- "@types/react": "18.2.37", - "@types/react-dom": "^18.2.0", + "@types/react": "^18.2.37", + "@types/react-dom": "^18.2.0",Or pin both:
- "@types/react": "18.2.37", - "@types/react-dom": "^18.2.0", + "@types/react": "18.2.37", + "@types/react-dom": "18.2.0",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/README.mdapps/assisted-ui/deploy/nginx.confapps/assisted-ui/package.jsonapps/assisted-ui/src/components/App.tsxapps/assisted-ui/src/components/Chatbot.tsxapps/assisted-ui/vite.config.tslibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/index.tslibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (5)
- apps/assisted-ui/vite.config.ts
- apps/assisted-ui/deploy/nginx.conf
- libs/ui-lib/lib/@types/global.d.ts
- apps/assisted-ui/README.md
- apps/assisted-ui/src/components/Chatbot.tsx
🚧 Files skipped from review as they are similar to previous changes (16)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
- apps/assisted-ui/package.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- libs/locales/tsconfig.json
- apps/assisted-chatbot/src/config.ts
- .eslintrc.json
- package.json
- libs/chatbot/package.json
- libs/ui-lib/vitest.config.ts
- apps/assisted-chatbot/tsconfig.json
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
- libs/ui-lib-tests/cypress/views/common.ts
- apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
- libs/types/tsconfig.json
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-ui/src/components/App.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
apps/assisted-ui/src/components/App.tsxapps/assisted-disconnected-ui/package.jsonlibs/ui-lib/package.json
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.json
🧬 Code graph analysis (2)
apps/assisted-ui/src/components/App.tsx (1)
libs/ui-lib/lib/ocm/components/Routes.tsx (1)
UILibRoutes(26-64)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
🔇 Additional comments (17)
libs/ui-lib/tsconfig.json (1)
20-20: LGTM!Enabling
skipLibCheckis a sensible addition that improves build performance by skipping type-checking of third-party.d.tsfiles. This aligns with common TypeScript best practices and is consistent with other library configs in the monorepo.apps/assisted-ui/src/components/App.tsx (1)
12-22: LGTM! Clean removal of ChatBot integration.The simplified
UILibRoutesusage correctly removes theadditionalComponentsprop as part of extracting the chatbot into the separateassisted-chatbotapp. The remaining routing structure and feature flags are properly preserved.apps/assisted-chatbot/.eslintrc.cjs (1)
1-39: LGTM!The ESLint configuration is well-structured with appropriate overrides for build config and source files. The
no-restricted-importsrule ensures consistentuseTranslationimports from the shared library.libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (2)
6-7: LGTM!Import paths correctly updated to
@patternfly/react-coreand@patternfly/react-iconsfor PatternFly 6.x compatibility.
49-70: LGTM!The
onFeedbackSubmitcallback correctly includesmessagescontext in dependencies (per project patterns) and properly handles API errors. The emptyuser_questionappears intentional for this feedback flow.libs/chatbot/lib/index.ts (1)
4-4: LGTM!New types
ApiCallInitandApiCallResponseare properly re-exported for external consumption.apps/assisted-chatbot/fec.config.js (1)
1-29: LGTM!The federation configuration is well-structured. The module name transformation, exposed components, and proxy settings are correctly configured for development.
apps/assisted-chatbot/package.json (1)
34-38: Consider aligning with ui-lib's more permissive versioning approach.The apps (assisted-chatbot, assisted-disconnected-ui, assisted-ui) all use identical versions (i18next ^20.4.0, react-i18next ^11.11.4), but libs/ui-lib uses more flexible ranges (i18next ^20.4 || ^21, react-i18next >11.7.3). This inconsistency suggests the apps may be intentionally constrained, but clarification is needed on whether they should adopt the library's strategy or if there's a specific compatibility reason for the pinning.
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
103-117: LGTM on the authentication-aware fetch wrapper.The
fetchFunctioncorrectly retrieves a fresh token on each request and properly spreads existing headers while conditionally adding the Authorization header. This ensures token refresh is handled per-request.
26-37: LGTM on error component.The
AsyncMessageErrorcomponent provides a user-friendly error message with proper structure for the chatbot UI.
100-159: The empty dependency array inuseMemois intentional.useChrome()from@redhat-cloud-services/frontend-componentsis a context hook that returns a stable reference across re-renders, making it safe to reference within the memoizedfetchFunctionwithout including it in the dependency array. This pattern is consistent throughout the codebase (seeuseIsAuthenticatedandChatbotMessageEntry.tsx), and the eslint-disable comment acknowledges the lint rule as intentionally suppressed for this known and accepted pattern.Likely an incorrect or invalid review comment.
libs/ui-lib/package.json (4)
99-100: Breaking change: React 17 support dropped.The peer dependencies for
reactandreact-domhave been narrowed from^17 || ^18to^18only. This is a breaking change for any consumers still on React 17.Ensure all downstream applications and consumers of
@openshift-assisted/ui-libhave been migrated to React 18 before this change is released.
104-104: LGTM on movingreact-router-dom-v5-compatto peerDependencies.Moving this package from dependencies to peerDependencies is the correct pattern for a shared library, as it allows consuming applications to control the version and avoids duplicate instances. Based on learnings,
react-router-dom-v5-compatis designed to work alongsidereact-router-domv4/v5 for incremental migration.
45-47: Type definitions aligned with React 18.The
@types/reactand@types/react-domversions are correctly aligned with the React 18 runtime peer dependencies.
6-12: @patternfly/react-core version 6.4.0 is not listed as the stable release on npmjs.com.PatternFly 6.4.0 was released in October 2025, but npmjs.com currently lists 6.3.1 as the latest stable version. While 6.4.0 artifacts exist on CDNs and prerelease builds are available, the official npm registry has not marked 6.4.0 as the latest stable release. Confirm that pinning 6.4.0 across all PatternFly packages will install from a prerelease or alternate source, or consider using 6.3.1 if this is not intentional.
apps/assisted-disconnected-ui/package.json (2)
24-25: LGTM!The change from
^5.3.3to5.3.xis functionally equivalent — both allow patch updates within the 5.3.x range. Based on learnings, havingreact-router-domv5 alongsidereact-router-dom-v5-compatis the correct and intended configuration for incremental migration.
4-11: PatternFly updates are consistent; dynamic-plugin-sdk 1.0.0 upgrade verified as compatible.The PatternFly packages are uniformly updated to 6.4.0, and
@openshift-console/dynamic-plugin-sdkis upgraded from 0.0.3 to 1.0.0. The codebase successfully uses v1.0.0 across the ui-lib shared library and multiple apps, with no breaking changes affecting current imports (K8sResourceCommon, Patch, K8sModel, and others remain compatible).
rawagner
force-pushed
the
chatbot_app
branch
2 times, most recently
from
128ddc8 to
d6c7041
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
72-72: Handle undefinedmessage.dategracefully.If
message.dateisundefined, the string interpolation produces"undefined undefined". Consider adding a fallback:🐛 Suggested fix
- const messageDate = `${message.date?.toLocaleDateString()} ${message.date?.toLocaleTimeString()}`; + const messageDate = message.date + ? `${message.date.toLocaleDateString()} ${message.date.toLocaleTimeString()}` + : '';
🤖 Fix all issues with AI agents
In @apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx:
- Around line 65-98: The useIsAuthenticated hook performs an async fetch inside
useEffect and may update state after the component unmounts; modify
useIsAuthenticated to create an AbortController, pass controller.signal to
fetch, and in the effect cleanup call controller.abort() so the request is
cancelled; after the fetch, only call setIsAuthenticated and setIsLoading if
!controller.signal.aborted (or catch DOMException for abort and skip state
updates), ensuring no state updates occur on an unmounted component.
- Around line 131-136: Update the StateManagerConfiguration type so
handleNewChat accepts async callbacks: change the handleNewChat signature in
types.ts (where StateManagerConfiguration is defined) to allow returning void or
Promise<void> — i.e., handleNewChat?: (toggleDrawer: (isOpen: boolean) => void)
=> void | Promise<void>; then remove the now-unnecessary
eslint-disable-next-line @typescript-eslint/no-misused-promises in
useAsyncChatbot.tsx where handleNewChat is implemented to keep type-safety
consistent.
🧹 Nitpick comments (12)
libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
94-97: Minor: Type assertion and redundant template literal.The explicit
stringtype onvalnarrows PatternFly's broaderstring | number | undefinedsignature—safe here since allSelectOptionvalues are strings. The template literal on line 95 is redundant.🔧 Optional simplification
- onSelect={(_, val: string) => { - setCategory(`${val || ''}`); + onSelect={(_, val) => { + setCategory(String(val ?? ''));libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx (1)
4-8: Consider explicitly typing thechildrenprop withReact.PropsWithChildren.In React 18,
React.FCno longer automatically includeschildrenin its type definition. While the current code functions, explicitly typing props withReact.PropsWithChildrenprovides better type safety and is a pattern already used throughout the codebase.♻️ Suggested fix
-const ClusterWizardStepHeader: React.FC = ({ children }) => ( +const ClusterWizardStepHeader: React.FC<React.PropsWithChildren> = ({ children }) => ( <Content data-testid="step-header" component="h2"> {children} </Content> );libs/ui-lib/package.json (1)
45-47: Inconsistent version specifier for @types/react.
@types/reactis pinned to an exact version18.2.37while@types/react-domuses a caret range^18.2.0. Consider using a consistent approach—either pin both or use ranges for both.Suggested fix for consistency
- "@types/react": "18.2.37", + "@types/react": "^18.2.37",apps/assisted-ui/package.json (1)
34-37: Verify type definition version compatibility.The type definitions use slightly different version specifier patterns (
18.2.37pinned vs^18.2.0with caret, and^5.1.xvs5.3.x). Consider aligning the specifier style for consistency, though this is minor.apps/assisted-chatbot/src/config.ts (2)
23-32: Validate localStorage value before use.If
envOverrideStorageItemcontains a stale or tampered value not present inenvs, the check on line 24 correctly prevents it from being used. However, the invalid value is then re-written to localStorage on line 26, which perpetuates the invalid state until a valid query param is provided.Consider clearing the invalid localStorage entry:
♻️ Suggested improvement
const queryEnv = parseEnvQueryParam() || envOverrideStorageItem; if (queryEnv && envs[queryEnv]) { try { localStorage.setItem(ENV_OVERRIDE_LOCALSTORAGE_KEY, queryEnv); } catch (e) { // eslint-disable-next-line no-console console.warn('failed to store override item', e); } return envs[queryEnv]; + } else if (envOverrideStorageItem && !envs[envOverrideStorageItem]) { + // Clear invalid stored value + try { + localStorage.removeItem(ENV_OVERRIDE_LOCALSTORAGE_KEY); + } catch (e) { + // eslint-disable-next-line no-console + console.warn('failed to remove invalid override item', e); + } }
33-37: Consider adding 'integration' to the hostname detection.The
envsmap includesintegration, but the hostname-based default selection only checks for 'dev' and 'foo'. If there's an integration environment hostname pattern, consider adding it here for completeness.libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (1)
128-133: Consider handling clipboard API errors.
navigator.clipboard.writeTextcan fail in non-secure contexts or when permissions are denied. The currentvoidusage silently swallows any rejection. Consider adding minimal error handling or user feedback for copy failures.♻️ Optional improvement
copy: { isDisabled: !message.answer, onClick: () => { - void navigator.clipboard.writeText(message.answer || ''); + navigator.clipboard.writeText(message.answer || '').catch((err) => { + // eslint-disable-next-line no-console + console.warn('Failed to copy to clipboard:', err); + }); }, },apps/assisted-chatbot/package.json (2)
5-8: Consider removing npm engine requirement.The repository uses Yarn as its package manager. The
npm >=7.0.0engine requirement may be unnecessary and could cause confusion.♻️ Suggested cleanup
"engines": { - "node": ">=16.0.0", - "npm": ">=7.0.0" + "node": ">=16.0.0" },
22-40: Dependencies are well-aligned with the broader PR changes.The PatternFly packages are consistently at 6.4.x, workspace dependency for
@openshift-assisted/chatbotenables local development, and the Red Hat Cloud Services packages provide the AI client functionality.Consider updating
parse-urlfrom ^9.2.0 to ^10.0.3 (latest available). The current version has no known vulnerabilities, but upgrading to the latest minor/major version aligns with best practices for package maintenance.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
39-63: Type mismatch forconversationIdand redundant fallback assignment.
The
conversationIdprop is typed asstring(required) on line 46, but theLightspeedMessagetype (line 22) declares it asstring | undefined. This mismatch could cause type errors at call sites.Setting
fallback: nullinmessageProps(line 53) is redundant since it's immediately overridden on line 60.Suggested fix
const LSCMessageEntry = ({ message, avatar, conversationId, }: { message: MessageType<LightSpeedCoreAdditionalProperties>; avatar: string; - conversationId: string; + conversationId: string | undefined; }) => { const messageProps: LightspeedMessage = { message, avatar: message.role === 'user' ? avatar : ARH_BOT_ICON, scope: 'assistedInstallerApp', module: './ChatbotMessageEntry', - fallback: null, conversationId, };
100-169: Manager created unconditionally despite authentication gating.The
useMemocreates the full configuration including theLightspeedClienteven when the user is not authenticated (lines 103-159). This work is wasted whenisAuthenticatedis false.Consider deferring manager creation until authentication is confirmed:
Suggested optimization
const useStateManager = (): UseManagerHook => { const chrome = useChrome(); const { isAuthenticated, isLoading } = useIsAuthenticated(); - const manager = React.useMemo(() => { + const manager = React.useMemo(() => { + if (!isAuthenticated) { + return null; + } const api = new URL(getBaseUrl()); // ... rest of manager setup return config; // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [isAuthenticated]); if (isLoading) { return { manager: null, loading: true }; } - if (!isAuthenticated) { - return { manager: null, loading: false }; - } - - return { manager, loading: false }; + return { manager, loading: false }; };apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (1)
11-22: Consider extracting the assisted-chat URL construction to a shared utility.The URL pattern
https://assisted-chat.${api.hostname}is duplicated here and inuseAsyncChatbot.tsx(lines 75 and 106). Extracting this toconfig.tswould centralize the URL logic and reduce duplication.Suggested approach in config.ts
// In config.ts export const getAssistedChatUrl = (): string => { const api = new URL(getBaseUrl()); return `https://assisted-chat.${api.hostname}`; };Then use
getAssistedChatUrl()in both files instead of constructing the URL inline.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/README.mdapps/assisted-ui/deploy/nginx.confapps/assisted-ui/package.jsonapps/assisted-ui/src/components/App.tsxapps/assisted-ui/src/components/Chatbot.tsxapps/assisted-ui/vite.config.tslibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxlibs/chatbot/lib/index.tslibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (5)
- apps/assisted-ui/README.md
- libs/ui-lib/lib/@types/global.d.ts
- apps/assisted-ui/vite.config.ts
- apps/assisted-ui/deploy/nginx.conf
- apps/assisted-ui/src/components/Chatbot.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
- .eslintrc.json
- libs/locales/tsconfig.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- libs/types/tsconfig.json
- libs/chatbot/lib/index.ts
- apps/assisted-disconnected-ui/package.json
- apps/assisted-chatbot/fec.config.js
- apps/assisted-chatbot/tsconfig.json
- package.json
- libs/ui-lib/tsconfig.json
- libs/chatbot/package.json
- libs/ui-lib/vitest.config.ts
- libs/ui-lib-tests/cypress/views/common.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
libs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/lib/components/ChatBot/MessageEntry.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-ui/src/components/App.tsxapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
apps/assisted-ui/src/components/App.tsxapps/assisted-ui/package.jsonlibs/ui-lib/package.json
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
apps/assisted-ui/package.jsonlibs/ui-lib/package.json
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
🧬 Code graph analysis (1)
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.tsx (1)
AsyncMessagePlaceholder(4-35)apps/assisted-chatbot/src/config.ts (1)
getBaseUrl(15-38)apps/assisted-chatbot/src/types.ts (2)
UseManagerHook(46-49)StateManagerConfiguration(27-44)
🔇 Additional comments (17)
apps/assisted-ui/src/components/App.tsx (1)
12-22: LGTM!The removal of the
additionalComponentsprop and chatbot-related wiring cleanly decouples the chatbot from this app, aligning with the PR objective of extracting it to a separate federated app. The remaining component structure is clean and functional.libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (2)
1-23: LGTM on PatternFly import migration.The import path updates from
@patternfly-6/*to@patternfly/*are correct for PatternFly v6 standard packages.
34-157: LGTM on component implementation.The form logic is well-structured: proper state management, focus handling via
useLayoutEffect, async submission with loading/error states, and accessible form controls with labels and IDs. Thevoid submit()pattern correctly handles the floating promise.libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM!Good refactoring to use the shared
ClusterWizardStepHeadercomponent. This aligns with the consistent header pattern being adopted across wizard steps (as seen in Day2StaticIP.tsx and StaticIpPage.tsx), and brings thedata-testidattribute for improved test reliability.Also applies to: 18-18
libs/ui-lib/package.json (3)
104-104: react-router-dom-v5-compat correctly moved to peerDependencies.Moving this package from dependencies to peerDependencies is appropriate and allows consumers to control the version. Both consuming apps (assisted-ui and assisted-disconnected-ui) have
react-router-dom-v5-compatinstalled, confirming proper configuration across the monorepo. This aligns with the package's design for incremental migration to React Router v6 APIs while maintaining v5 compatibility.
99-100: Verify the scope of React 18-only peer dependency requirement.The current
peerDependenciesin libs/ui-lib/package.json specifies"react": "^18"(without^17support). Unable to verify the original specification was"^17 || ^18"from git history. If this is intentional and represents a breaking change from previous versions, ensure the major version bump and changelog entry are in place. No other packages in the monorepo depend on React 17, but external consumers on React 17 will be affected.
6-12: PatternFly upgrade to 6.4.0 is approved.Minor version bump from 6.2.x to 6.4.0 across all PatternFly packages. PatternFly's release policy ensures that minor (6.x) releases contain no planned breaking changes — only major releases introduce breaking changes. Pinned versions maintain consistency across the PatternFly suite, which is appropriate for a library.
apps/assisted-chatbot/.eslintrc.cjs (1)
1-39: LGTM!The ESLint configuration is well-structured with appropriate overrides for build tooling (vite.config.ts) and source files. The
no-restricted-importsrule enforcing the use ofuseTranslationfrom@openshift-assisted/ui-lib/commonensures consistency across the chatbot app.apps/assisted-ui/package.json (1)
4-11: PatternFly dependencies consistently upgraded to 6.4.0.All PatternFly packages are aligned at version 6.4.0, ensuring compatibility across the component library.
libs/chatbot/lib/components/ChatBot/MessageEntry.tsx (2)
6-7: LGTM - PatternFly import migration.The imports have been correctly updated from
@patternfly-6/to@patternfly/namespace, aligning with the 6.4.0 version across the project.
19-30: Well-designed API abstraction types.The inline
ApiCallInitandApiCallResponsetypes provide a clean abstraction over the fetch API, avoiding dependency on DOM globals. This makes the component more portable across different build environments.apps/assisted-chatbot/package.json (1)
39-39: Verifyreact-router-dom-v5-compatusage withoutreact-router-dom.This package includes
react-router-dom-v5-compatbut notreact-router-domas a direct dependency. This may be intentional for a federated module that receives routing context from a host application, but verify this is the intended configuration.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (3)
1-16: LGTM!Imports are well-organized with external dependencies first, followed by internal types and components.
17-24: LGTM!The type definition correctly captures the ScalprumComponent props shape for the Lightspeed message entry.
26-37: LGTM!Clean error component providing user-friendly fallback when message loading fails.
apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx (2)
1-5: LGTM!Clean import structure with external dependencies followed by internal config.
24-31: LGTM!The
openClusterDetailscallback correctly includeschromeHistoryin its dependency array, and the navigation path follows a clear pattern. The component properly forwards remaining props toMessageEntry.
rawagner
force-pushed
the
chatbot_app
branch
2 times, most recently
from
eb9af90 to
e47c94c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/assisted-ui/package.json:
- Line 36: Replace the non-standard version specifier for the
@types/react-router dependency ("^5.1.x") with a valid semver form; update the
dependency entry for @types/react-router to either a caret-prefixed full version
like "^5.1.0" (preferred) or use a wildcard form without a caret like "5.1.x" to
avoid ambiguous behavior.
🧹 Nitpick comments (6)
libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
94-97: Consider using the correct type foronSelectvalue parameter.PatternFly's
SelectonSelectcallback types the value asstring | number | undefined, not juststring. While the current code works at runtime (since allSelectOptionvalues are strings and the fallback handlesundefined), the explicitstringtype annotation suppresses TypeScript's type checking.♻️ Suggested improvement
- onSelect={(_, val: string) => { - setCategory(`${val || ''}`); + onSelect={(_, val) => { + setCategory(typeof val === 'string' ? val : '');Alternatively, if you prefer to keep the simpler template literal approach, just remove the explicit type annotation and let TypeScript infer it:
- onSelect={(_, val: string) => { + onSelect={(_, val) => {libs/ui-lib-tests/cypress/views/common.ts (1)
80-82: Consider consistency with other selector methods.
verifyIsAtSubStep(line 81) still usescy.get('h3')andvisitClusterDetailsPage(line 106) usescy.get('h2'). If the header migration toClusterWizardStepHeaderextends to sub-step headers or other pages, these selectors may become fragile.Consider whether these methods should also adopt
data-testidselectors for consistency.Also applies to: 104-107
apps/assisted-ui/package.json (1)
34-37: Inconsistent version specifier patterns in type definitions.The type definitions use a mix of pinning strategies:
- Line 34: exact pin (
18.2.37)- Line 35: caret range (
^18.2.0)- Line 36: mixed syntax (
^5.1.x)- Line 37: x wildcard (
5.3.x)Consider standardizing the approach for maintainability. Caret ranges (
^) are the npm default and provide a good balance of flexibility and stability.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
48-61: RedundantfallbackinmessageProps.Line 53 sets
fallback: null, but this is immediately overridden by line 60'sfallback={<AsyncMessagePlaceholder />}. Consider removing the redundant property frommessageProps.🧹 Suggested cleanup
const messageProps: LightspeedMessage = { message, avatar: message.role === 'user' ? avatar : ARH_BOT_ICON, scope: 'assistedInstallerApp', module: './ChatbotMessageEntry', - fallback: null, conversationId, };
65-98: Consider handling component unmount during async operation.The async effect fetches data and updates state without handling component unmount. If the component unmounts while the fetch is in-flight,
setIsAuthenticatedandsetIsLoadingwill be called on an unmounted component.Based on learnings from this repository, consider using
useStateSafelyor anAbortControllerwith cleanup to prevent state updates on unmounted components.♻️ Example using AbortController
const useIsAuthenticated = () => { const chrome = useChrome(); const [isLoading, setIsLoading] = React.useState(true); const [isAuthenticated, setIsAuthenticated] = React.useState(false); React.useEffect(() => { + const abortController = new AbortController(); void (async () => { const api = new URL(getBaseUrl()); const token = await chrome.auth.getToken(); try { const response = await fetch(`https://assisted-chat.${api.hostname}/v1/conversations`, { method: 'GET', headers: token ? { Authorization: `Bearer ${token}`, } : {}, + signal: abortController.signal, }); + if (abortController.signal.aborted) return; setIsAuthenticated(response.ok); } catch (error) { + if (abortController.signal.aborted) return; // eslint-disable-next-line no-console console.error('Error checking authentication:', error); } finally { + if (!abortController.signal.aborted) { setIsLoading(false); + } } })(); + return () => abortController.abort(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); return { isAuthenticated, isLoading, }; };libs/ui-lib/package.json (1)
45-47: Inconsistent version specifier for@types/react.
@types/reactis pinned to exact version18.2.37without a caret or tilde, while@types/react-domuses^18.2.0. This inconsistency may cause type mismatches if@types/react-domupdates independently. Consider using^18.2.37for consistency, or pin both to exact versions if there's a specific reason for the exact pin.Suggested fix
- "@types/react": "18.2.37", + "@types/react": "^18.2.37",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.eslintrc.jsonapps/assisted-chatbot/.eslintrc.cjsapps/assisted-chatbot/fec.config.jsapps/assisted-chatbot/package.jsonapps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scssapps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsxapps/assisted-chatbot/src/config.tsapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxapps/assisted-chatbot/tsconfig.jsonapps/assisted-disconnected-ui/package.jsonapps/assisted-ui/README.mdapps/assisted-ui/deploy/nginx.confapps/assisted-ui/package.jsonapps/assisted-ui/src/components/App.tsxapps/assisted-ui/src/components/Chatbot.tsxapps/assisted-ui/vite.config.tslibs/chatbot/lib/components/ChatBot/FeedbackCard.tsxlibs/chatbot/package.jsonlibs/locales/tsconfig.jsonlibs/types/tsconfig.jsonlibs/ui-lib-tests/cypress/views/common.tslibs/ui-lib/lib/@types/global.d.tslibs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsxlibs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsxlibs/ui-lib/package.jsonlibs/ui-lib/tsconfig.jsonlibs/ui-lib/vitest.config.tspackage.json
💤 Files with no reviewable changes (5)
- apps/assisted-ui/vite.config.ts
- libs/ui-lib/lib/@types/global.d.ts
- apps/assisted-ui/src/components/Chatbot.tsx
- apps/assisted-ui/deploy/nginx.conf
- apps/assisted-ui/README.md
🚧 Files skipped from review as they are similar to previous changes (16)
- .eslintrc.json
- apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.scss
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx
- apps/assisted-chatbot/package.json
- libs/ui-lib/tsconfig.json
- apps/assisted-chatbot/.eslintrc.cjs
- libs/ui-lib/lib/ocm/components/AddHosts/day2Wizard/Day2StaticIP.tsx
- apps/assisted-chatbot/tsconfig.json
- package.json
- libs/ui-lib/lib/common/components/clusterWizard/ClusterWizardStepHeader.tsx
- libs/chatbot/package.json
- apps/assisted-chatbot/fec.config.js
- libs/ui-lib/vitest.config.ts
- apps/assisted-disconnected-ui/package.json
- apps/assisted-chatbot/src/components/ChatbotMessageEntry/ChatbotMessageEntry.tsx
- libs/locales/tsconfig.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
Repo: openshift-assisted/assisted-installer-ui PR: 3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
apps/assisted-ui/src/components/App.tsxapps/assisted-chatbot/src/hooks/useAsyncChatbot.tsxlibs/chatbot/lib/components/ChatBot/FeedbackCard.tsx
📚 Learning: 2026-01-07T15:19:02.310Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 3350
File: apps/assisted-ui/package.json:26-27
Timestamp: 2026-01-07T15:19:02.310Z
Learning: The react-router-dom-v5-compat package is designed to work alongside react-router-dom v4 or v5, not replace it. It declares peer dependencies on react-router-dom "4 || 5" and allows incremental migration by providing React Router v6 APIs while the application continues to run on v5. Having both react-router-dom5.x and react-router-dom-v5-compat installed together is the correct and intended configuration.
Applied to files:
apps/assisted-ui/src/components/App.tsxlibs/ui-lib/package.jsonapps/assisted-ui/package.json
📚 Learning: 2025-04-14T09:11:08.834Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.834Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
Applied to files:
apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx
📚 Learning: 2026-01-05T14:40:12.986Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3348
File: libs/ui-lib/lib/common/components/ui/PrismCode.tsx:10-16
Timestamp: 2026-01-05T14:40:12.986Z
Learning: The assisted-installer-ui repository does not use server-side rendering (SSR), so window and document access guards are not required in UI components like PrismCode.
Applied to files:
libs/ui-lib/package.jsonapps/assisted-ui/package.json
🔇 Additional comments (13)
libs/types/tsconfig.json (1)
7-7: LGTM!Adding
skipLibCheck: trueis a reasonable approach given the dependency upgrades in this PR. This prevents type-checking conflicts from third-party declaration files while maintaining type safety in your own code. The change is consistent with similar configurations applied to other libs in this PR.apps/assisted-ui/src/components/App.tsx (1)
12-22: LGTM! Clean removal of chatbot integration.The simplification of
UILibRoutesby removing theadditionalComponentsprop aligns with the PR objective of extracting the chatbot into a standalone federated app. The remaining routing structure and component hierarchy are intact.libs/chatbot/lib/components/ChatBot/FeedbackCard.tsx (1)
1-23: LGTM on import path updates.The import paths correctly align with PatternFly 6's standard package naming convention (
@patternfly/react-coreand@patternfly/react-icons).libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/CustomManifestsPage.tsx (1)
7-7: LGTM!Clean refactoring to use the centralized
ClusterWizardStepHeadercomponent, which aligns with the consistent header pattern being adopted across other cluster configuration pages (Day2StaticIP, StaticIpPage). Thedata-testidsupport enables reliable test selectors.Also applies to: 17-18
libs/ui-lib-tests/cypress/views/common.ts (1)
77-79: Good improvement usingdata-testidselector.Using
data-testidattributes is a best practice for test stability. However, the timeout increase from 3s to 10s is significant (over 3x). Please verify this is intentional and not masking a performance regression.apps/assisted-ui/package.json (2)
24-25: Router configuration is correct for incremental migration.Based on learnings, having both
react-router-domv5.3.x andreact-router-dom-v5-compatinstalled together is the intended configuration for incremental migration to React Router v6.
4-11: PatternFly pinned to 6.4.0, which is only available as prerelease—not a stable release.The package.json pins all PatternFly packages to version 6.4.0, but web searches confirm this version is not published as "latest" on npm.org. Only 6.4.0-prerelease versions currently exist; the latest stable release is 6.3.1. Pinning to a prerelease version could cause unexpected behavior during installation or CI/CD workflows and should either be downgraded to the stable 6.3.1 or justified if a prerelease is intentional.
Likely an incorrect or invalid review comment.
apps/assisted-chatbot/src/config.ts (1)
1-38: LGTM!The environment resolution logic is well-structured with proper error handling around localStorage operations. The priority chain (query param → localStorage → hostname-based default) is clear and the validation against known
envskeys prevents invalid URLs.Minor observation: the constant
ocmOverridenEnvironmenthas a typo ("Overriden" vs "Overridden"), but this is non-breaking since it's used consistently.apps/assisted-chatbot/src/hooks/useAsyncChatbot.tsx (2)
100-169: Reasonable hook structure.The hook correctly separates concerns between authentication checking and state manager creation. The conditional return pattern (lines 161-168) properly gates the manager based on authentication state.
Note: The
LightspeedClientandstateManagerare created eagerly viauseMemoeven when authentication hasn't completed yet. If initialization cost becomes a concern, consider deferring creation untilisAuthenticatedis confirmed. For now, this is likely acceptable.
26-37: LGTM!The error component provides clear user feedback with appropriate severity.
libs/ui-lib/package.json (3)
104-104: LGTM:react-router-dom-v5-compatpeer dependency.Adding
react-router-dom-v5-compatas a peer dependency is correct for incremental migration. Based on learnings, this package is designed to work alongsidereact-router-domv4/v5, providing React Router v6 APIs while the application continues to run on v5.
99-100: Verify the breaking change claim before documenting.The current peer dependency is correctly set to
"^18"for bothreactandreact-dom. However, the claim that this was "narrowed from^17 || ^18" cannot be verified from the git history—the file appears to have been created with^18only. Before documenting this as a breaking change, confirm with the team:
- Was the previous version actually
^17 || ^18?- If this is a breaking change, add it to release notes and migration guides.
- If React 17 support was already dropped earlier (e.g., during the React 18 migration), clarify the timeline.
No packages in the monorepo currently require React 17, so the impact is isolated to external consumers.
6-12: PatternFly 6.4.0 is a stable minor release with no breaking changes.The upgrade from 6.2.x to 6.4.0 is safe—PatternFly 6.4 is a quarterly minor release featuring new functionality and accessibility improvements, with no reported breaking changes.
apps/assisted-chatbot/src/components/AsyncMessagePlaceholder/AsyncMessagePlaceholder.css
Show resolved
Hide resolved
openshift-ci-robot
added
the
jira/valid-reference
|
@rawagner: This pull request references MGMT-22746 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, rawagner 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 |
openshift-merge-bot
bot
merged commit 8bb681c
into
openshift-assisted:master
3 participants
This is a separate app for AI chabot, that is currently part of https://github.com/openshift-assisted/assisted-installer-app
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.