MGMT-20953: Add AI ChatBot#3016
MGMT-20953: Add AI ChatBot#3016openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
approved
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatBot
participant ChatBotWindow
participant API
User->>ChatBot: Clicks Toggle Button
ChatBot->>ChatBotWindow: Renders if open
User->>ChatBotWindow: Sends message
ChatBotWindow->>API: Calls onApiCall (streaming)
API-->>ChatBotWindow: Streams response tokens
ChatBotWindow->>ChatBotWindow: Updates messages, handles streaming
ChatBotWindow-->>User: Renders updated conversation
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
libs/chatbot/.eslintrc.cjsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/chatbot/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. libs/chatbot/lib/components/ChatBot/ChatBot.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/chatbot/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. libs/chatbot/lib/components/ChatBot/Chatbot.cssOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/chatbot/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
rawagner
force-pushed
the
chatbot
branch
2 times, most recently
from
a5b66e8 to
4a08b93
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
1-10: Minor ordering / readability nitConsider alphabetising the export list while touching this file; keeping barrel exports sorted eases scanning and merges.
libs/chatbot/tsconfig.eslint.json (1)
1-5: Lint config: exclude build artifactsRight now
excludeis an empty array, soeslint --ext .tswill crawl any generated files placed inside this package (e.g.dist,coverage).
Add the usual exclusions to keep linting fast and deterministic."include": ["lib", "vitest.config.ts"], - "exclude": [] + "exclude": ["dist", "coverage", "node_modules"]libs/chatbot/lib/typings.d.ts (1)
1-4: Broaden SVG typing to support ReactComponent importsMany loaders expose both a URL string and a React component (
import { ReactComponent as Icon } from './icon.svg').
If that pattern is (or might become) necessary, extend the ambient module:declare module '*.svg' { - const content: string; - export default content; + import * as React from 'react'; + const src: string; + export default src; + export const ReactComponent: React.FC<React.SVGProps<SVGSVGElement>>; }libs/chatbot/lib/components/Chatbot/Chatbot.css (1)
1-14: Extremely high z-index may collide with other overlays
z-index: 29999will override PatternFly modals, tooltips, and potentially 3rd-party overlays.
Consider adding the chatbot atvar(--pf-c-modal-box--ZIndex)+ 1 or using a dedicated z-index layer token.Also, the fixed positioning ignores viewport safe-area insets (mobile notch / dynamic island). If mobile support is planned, add
env(safe-area-inset-*)paddings.libs/ui-lib/lib/ocm/components/Routes.tsx (1)
22-22: Consider the prop naming and placement.The
additionalComponentsprop name is generic. Consider a more specific name likeoverlayComponentsorwidgetComponentsto better convey intent.Also applies to: 28-28
apps/assisted-ui/src/components/App.tsx (1)
9-14: Consider optimizing CSS imports to reduce bundle size.Multiple PatternFly 6 CSS files are being imported, which could significantly increase the bundle size. Consider:
- Importing only the specific CSS modules needed by the chatbot
- Using CSS tree-shaking if available
- Evaluating if all these imports are necessary
libs/chatbot/lib/components/Chatbot/ChatBot.tsx (1)
8-16: Consider enhancing accessibility and error handling.The basic implementation works but could benefit from:
- Keyboard navigation support (ESC to close)
- Focus management when toggling visibility
- Error boundary to handle ChatBotWindow failures gracefully
- ARIA attributes for screen readers
const ChatBot = () => { const [chatbotVisible, setChatbotVisible] = React.useState<boolean>(false); + + React.useEffect(() => { + const handleEscape = (event: KeyboardEvent) => { + if (event.key === 'Escape' && chatbotVisible) { + setChatbotVisible(false); + } + }; + + document.addEventListener('keydown', handleEscape); + return () => document.removeEventListener('keydown', handleEscape); + }, [chatbotVisible]); return ( - <div id="chatbot-overlay" className="ai-chatbot"> + <div + id="chatbot-overlay" + className="ai-chatbot" + role="region" + aria-label="AI Chatbot" + > <ChatBotToggle chatbotVisible={chatbotVisible} setChatbotVisible={setChatbotVisible} /> {chatbotVisible && <ChatBotWindow />} </div> ); };libs/chatbot/lib/components/Chatbot/ChatBotWindow.tsx (2)
26-30: Replace weak ID generation with a more robust approach.The current ID generation using
Date.now() + Math.random()could theoretically produce collisions, though unlikely in practice. Consider using a more robust approach likecrypto.randomUUID()or a proper UUID library.- // you will likely want to come up with your own unique id function; this is for demo purposes only - const generateId = () => { - const id = Date.now() + Math.random(); - return id.toString(); - }; + const generateId = () => { + return crypto.randomUUID(); + };
100-109: Optimize rendering logic to prevent unnecessary re-renders.The current rendering logic creates a fragment for the last message, which could be optimized.
{messages.map((message, index) => { - const msg = <Message key={message.id} {...message} />; - - if (index === messages.length - 1) { - return ( - <> - <div ref={scrollToBottomRef} /> - {msg} - </> - ); - } - return msg; + const isLastMessage = index === messages.length - 1; + return ( + <React.Fragment key={message.id}> + {isLastMessage && <div ref={scrollToBottomRef} />} + <Message {...message} /> + </React.Fragment> + ); })}package.json (1)
27-27: Consider improving error handling in build script.The current build script chains commands with
&&, which means if the first build fails, the second won't run. This might be the desired behavior, but consider whether parallel builds or better error reporting would be beneficial.For better error handling and potentially faster builds, consider:
- "_build:lib": "yarn workspace @openshift-assisted/ui-lib build && yarn workspace @openshift-assisted/chatbot build", + "_build:lib": "yarn workspaces foreach --from '@openshift-assisted/{ui-lib,chatbot}' run build",This uses yarn's native workspace commands which provide better error reporting and can run in parallel.
libs/chatbot/package.json (1)
17-18: Consider cross-platform compatibility for asset copying.The asset copying scripts use
rsync, which might not be available on all development environments (particularly Windows).Consider using a Node.js-based solution for better cross-platform compatibility:
- "copy:svg": "rsync -Ruv lib/./**/*.svg build/cjs", - "copy:css": "rsync -Ruv lib/./**/*.css build/cjs", + "copy:svg": "node -e \"require('fs').cpSync('lib', 'build/cjs', {recursive: true, filter: (src) => src.endsWith('.svg')})\"", + "copy:css": "node -e \"require('fs').cpSync('lib', 'build/cjs', {recursive: true, filter: (src) => src.endsWith('.css')})\"",Or use a dedicated cross-platform tool like
cpy-cli:+ "cpy-cli": "^4.0.0",- "copy:svg": "rsync -Ruv lib/./**/*.svg build/cjs", - "copy:css": "rsync -Ruv lib/./**/*.css build/cjs", + "copy:svg": "cpy 'lib/**/*.svg' build/cjs --parents", + "copy:css": "cpy 'lib/**/*.css' build/cjs --parents",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
libs/chatbot/lib/assets/avatarimg.svgis excluded by!**/*.svglibs/chatbot/lib/assets/rh-logo.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
apps/assisted-ui/package.json(1 hunks)apps/assisted-ui/src/components/App.tsx(1 hunks)apps/assisted-ui/tsconfig.json(1 hunks)libs/chatbot/.eslintrc.cjs(1 hunks)libs/chatbot/lib/components/Chatbot/ChatBot.tsx(1 hunks)libs/chatbot/lib/components/Chatbot/ChatBotToggle.tsx(1 hunks)libs/chatbot/lib/components/Chatbot/ChatBotWindow.tsx(1 hunks)libs/chatbot/lib/components/Chatbot/Chatbot.css(1 hunks)libs/chatbot/lib/index.ts(1 hunks)libs/chatbot/lib/typings.d.ts(1 hunks)libs/chatbot/package.json(1 hunks)libs/chatbot/tsconfig.eslint.json(1 hunks)libs/chatbot/tsconfig.json(1 hunks)libs/chatbot/vitest.config.ts(1 hunks)libs/ui-lib/lib/common/features/featureGate.tsx(1 hunks)libs/ui-lib/lib/ocm/components/Routes.tsx(2 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: lint
🔇 Additional comments (11)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
9-9: Barrel export looks good – double-check hook’s export style
useFeatureis re-exported as a named export, whereas the other hooks are re-exported as default exports.
Ensurelibs/ui-lib/lib/ocm/hooks/use-feature.tsactually definesexport const useFeature = …(notexport default …) to avoid runtime undefined errors.Run a quick grep to confirm:
#!/bin/bash ast-grep --pattern $'export const useFeature = $_' | head -n 10apps/assisted-ui/tsconfig.json (1)
8-10: LGTM! Clean TypeScript project reference integration.The addition of the chatbot library reference follows the established pattern and enables proper TypeScript composite builds.
libs/ui-lib/lib/common/features/featureGate.tsx (1)
4-6: LGTM! Proper feature type extension.The addition of the
'assisted-chatbot'feature type correctly extends the feature gating system and follows the established pattern.libs/chatbot/vitest.config.ts (1)
1-15: Excellent Vitest configuration for React component testing.The configuration is well-structured with appropriate choices:
happy-domenvironment is lightweight and suitable for React testing- Inlining PatternFly dependencies avoids potential module resolution issues
- Module resolution prioritizes modern ES module formats
- Setup file enables proper test configuration
apps/assisted-ui/package.json (2)
3-3: LGTM: Workspace dependency addition.The chatbot workspace dependency is correctly configured for the monorepo structure.
6-11: ```shell
#!/bin/bash
set -eLocate the assisted-ui package.json
pkg=$(fd "apps/assisted-ui/package.json" -t f | head -1)
echo "Found package file: $pkg"echo -e "\n=== dependencies ==="
sed -n '1,200p' "$pkg" | sed -n '/"dependencies"/,/}/p'echo -e "\n=== devDependencies ==="
sed -n '1,200p' "$pkg" | sed -n '/"devDependencies"/,/}/p'echo -e "\n=== Search for PatternFly v5 imports in source ==="
rg -n "@patternfly/react-(core|styles|tokens|icons)" -n apps/assisted-ui/src || echo "No PF5 imports found."echo -e "\n=== Search for PatternFly alias usage ==="
rg -n "@patternfly-6/" -n apps/assisted-ui/src || echo "No PF6 aliases found."</details> <details> <summary>libs/chatbot/.eslintrc.cjs (1)</summary> `1-27`: **LGTM: Well-structured ESLint configuration.** The configuration appropriately separates concerns between test configuration and library code, with sensible overrides for the Vitest environment. </details> <details> <summary>libs/chatbot/lib/components/Chatbot/ChatBotToggle.tsx (1)</summary> `1-20`: **LGTM: Clean wrapper component implementation.** The component provides proper TypeScript typing and correctly abstracts the PatternFly 6 ChatbotToggle with appropriate prop mapping and event handling. </details> <details> <summary>libs/chatbot/tsconfig.json (1)</summary> `1-36`: **LGTM: Well-configured TypeScript setup for library.** The configuration properly sets up composite builds with declaration generation and appropriate project references for the monorepo structure. </details> <details> <summary>package.json (1)</summary> `28-28`: **Verify cross-platform compatibility of bash command.** The `_yalc:push` script uses bash-specific syntax which might not work on Windows systems without bash. Please verify that the bash command works across all development environments: ```shell #!/bin/bash # Check if the bash command syntax works on the current system echo "Testing bash command compatibility..." bash -c "for lib in ui-lib types locales chatbot; do echo \"Processing: \${lib}\"; done"libs/chatbot/package.json (1)
24-31: Verify PatternFly 6 version compatibility and consider version alignment.The PatternFly 6 packages use npm aliases pointing to specific versions. Ensure these versions are compatible with each other and consider whether they should be aligned.
Please verify that all PatternFly 6 packages are compatible:
Are PatternFly packages @patternfly/chatbot@2.2.1, @patternfly/patternfly@6.2.2, @patternfly/react-core@6.2.2, @patternfly/react-icons@6.2.2, @patternfly/react-styles@6.2.2, and @patternfly/react-tokens@6.2.2 compatible with each other?
openshift-ci
bot
added
the
do-not-merge/work-in-progress
rawagner
force-pushed
the
chatbot
branch
4 times, most recently
from
761d5c9 to
310996a
Compare
There's two PRs that haven't been merged yet, but are useful: lightspeed-core/lightspeed-stack#163 (tl;dr - conversation_id retention fix) openshift-assisted/assisted-installer-ui#3016 (tl;dr - draft UI for the chatbot) This commit updates the submodules to checkout those PRs
There's two PRs that haven't been merged yet, but are useful: lightspeed-core/lightspeed-stack#163 (tl;dr - conversation_id retention fix) openshift-assisted/assisted-installer-ui#3016 (tl;dr - draft UI for the chatbot) This commit updates the submodules to checkout those PRs
There's two PRs that haven't been merged yet, but are useful: lightspeed-core/lightspeed-stack#163 (tl;dr - conversation_id retention fix) openshift-assisted/assisted-installer-ui#3016 (tl;dr - draft UI for the chatbot) This commit updates the submodules to checkout those PRs Also updates the assisted-chat-pod.yaml to use correctly set up the UI pod so it's accessible at http://localhost:8080 and can communicate with the lightspeed-stack container. This still doesn't fully work because the proxy in the container cannot be configured to use the token / URL yet
rawagner
force-pushed
the
chatbot
branch
3 times, most recently
from
836f7aa to
9fe1efa
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: celdrake, 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 |
|
@rawagner: This pull request references MGMT-20953 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.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-merge-bot
bot
merged commit 9a3fb5c
into
openshift-assisted:master
|
/cherry-pick releases/v2.43 |
|
@rawagner: new pull request created: #3036 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Summary by CodeRabbit
New Features
Style
Chores