MGMT-21375: Show link to cluster details on cluster install action#3110
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds an openClusterDetails callback threaded from the app into ChatBot → ChatBotWindow → BotMessage, introduces a MsgAction type carrying optional url or clusterId and maps install/create cluster tool responses to clusterId actions, and removes credentials:'include' from the streaming fetch. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App (Chat UI)
participant ChatBot
participant ChatBotWindow
participant BotMessage
participant Router
User->>App: Open chat / interact
App->>ChatBot: render with props { onApiCall, username, openClusterDetails }
ChatBot->>ChatBotWindow: forward props
ChatBotWindow->>BotMessage: render message with actions (url? / clusterId?)
User->>BotMessage: Click "Open cluster details page"
BotMessage->>Router: openClusterDetails(clusterId)
Router-->>User: Navigate to /assisted-installer/clusters/{id}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
approved
rawagner
force-pushed
the
chatbot_details_link
branch
from
a45f9c0 to
edae9a7
Compare
rawagner
changed the title
|
@rawagner: This pull request references MGMT-21375 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "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
bot
removed
the
size/XXL
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
size/M
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@rawagner: This pull request references MGMT-21375 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
26-39: Make openClusterDetails optional with a no-op default to preserve API compatibilityThis avoids a breaking change for consumers that don’t need cluster navigation.
export type ChatBotWindowProps = { error?: string; resetError: VoidFunction; conversationId: string | undefined; messages: MsgProps[]; onApiCall: typeof fetch; username: string; onClose: () => void; sentMessage: (msg: string) => Promise<unknown>; startNewConversation: VoidFunction; isStreaming: boolean; announcement: string | undefined; - openClusterDetails: (clusterId: string) => void; + openClusterDetails?: (clusterId: string) => void; };And in the component parameters:
- openClusterDetails, + openClusterDetails = () => undefined,libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
115-147: Prevent rendering a stray “0” when actions is an empty arrayUsing && with a numeric length can render 0. Coerce to boolean.
- {!isLoading && message.actions?.length && ( + {!isLoading && !!message.actions?.length && ( <Stack hasGutter> {message.actions.map(({ title, url, clusterId }, idx) => ( <StackItem key={idx}> {url && ( <Button onClick={() => { try { saveAs(url); } catch (error) { // eslint-disable-next-line console.error('Download failed: ', error); } }} variant="secondary" icon={<DownloadIcon />} + aria-label={title} > {title} </Button> )} {clusterId && ( <Button onClick={() => openClusterDetails(clusterId)} variant="secondary" icon={<ExternalLinkAltIcon />} + aria-label={title} > {title} </Button> )} </StackItem> ))} </Stack> )}
🧹 Nitpick comments (4)
libs/chatbot/lib/components/ChatBot/helpers.ts (1)
53-55: Internationalize user-facing stringsWrap labels in your i18n util so they can be localized.
Example:
- 'Download ISO'
Download ${args?.file_name || 'credentials'}- 'Open cluster details page'
Also applies to: 71-73, 79-81
apps/assisted-ui/src/components/Chatbot.tsx (1)
51-60: Stabilize onApiCall with useCallbackPrevents unnecessary re-renders of children relying on referential equality. No behavior change.
- const onApiCall: ChatBotWindowProps['onApiCall'] = async (input, init) => { + const onApiCall: ChatBotWindowProps['onApiCall'] = React.useCallback(async (input, init) => { const token = await getOcmToken(); return fetch(`/chatbot${input.toString()}`, { ...(init || {}), headers: { ...(init?.headers || {}), Authorization: `Bearer ${token}`, }, }); - }; + }, []);libs/chatbot/lib/components/ChatBot/BotMessage.tsx (2)
7-7: Standardize icon imports to avoid bundle duplicationOther files import icons from '/dist/js/icons/...'. Align to one style to prevent multiple icon builds.
-import { DownloadIcon, ExternalLinkAltIcon } from '@patternfly-6/react-icons'; +import { DownloadIcon } from '@patternfly-6/react-icons/dist/js/icons/download-icon'; +import { ExternalLinkAltIcon } from '@patternfly-6/react-icons/dist/js/icons/external-link-alt-icon';
117-145: Consider deterministic keys and mutually exclusive actions
- Use a stable key (e.g., title) instead of the array index if possible.
- If an item ever contains both url and clusterId, you’ll render two buttons; confirm this is intended. If not, prefer else-if.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/assisted-ui/src/components/Chatbot.tsx(3 hunks)libs/chatbot/lib/components/ChatBot/BotMessage.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBot.tsx(2 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(3 hunks)libs/chatbot/lib/components/ChatBot/helpers.ts(3 hunks)libs/chatbot/lib/hooks/use-message.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libs/chatbot/lib/hooks/use-message.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
PR: openshift-assisted/assisted-installer-ui#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/ChatBot.tsx
🧬 Code Graph Analysis (1)
libs/chatbot/lib/components/ChatBot/ChatBot.tsx (2)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
ChatBotWindowProps(26-39)libs/chatbot/lib/index.ts (1)
ChatBotWindowProps(2-2)
🔇 Additional comments (5)
apps/assisted-ui/src/components/Chatbot.tsx (1)
2-2: Router compat import LGTMuseNavigate from react-router-dom-v5-compat is appropriate for v5-compatible navigation.
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
150-151: Prop plumbing to BotMessage looks correctProp is forwarded to BotMessage where it’s used. Once the prop is made optional, the default will keep this safe.
libs/chatbot/lib/components/ChatBot/ChatBot.tsx (2)
25-33: Forward optional openClusterDetails safelyIf left undefined by consumers, ChatBotWindow will use its no-op default.
5-5: Import path change for useMessages is fineNo functional impact. Just ensure tree-shaking doesn’t duplicate hook code.
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
45-67: Feedback submit hook dependencies look goodIncluding message and userMsg in the dependency array matches prior guidance to avoid stale closures.
rawagner
force-pushed
the
chatbot_details_link
branch
from
edae9a7 to
801a447
Compare
|
/cherry-pick releases/v0.1-chatbot |
|
@rawagner: once the present PR merges, I will cherry-pick it on top of 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. |
|
@rawagner: This pull request references MGMT-21375 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "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. |
rawagner
force-pushed
the
chatbot_details_link
branch
from
801a447 to
e8eec16
Compare
|
@rawagner: This pull request references MGMT-21375 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82, 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
libs/chatbot/lib/components/ChatBot/helpers.ts (2)
4-4: Export MsgAction type to avoid leaking private typeThe
MsgActiontype is used in the exportedMsgPropsinterface but is not exported itself, which can cause issues for consumers trying to type actions without relying on internal implementation details.
76-83: Fix switch statement fall-throughThe
install_clustercase is missing abreakorreturnstatement, causing it to fall through to thecreate_clustercase when the condition isn't met.libs/chatbot/lib/components/ChatBot/ChatBot.tsx (1)
9-9: Make openClusterDetails optional to avoid breaking changeThe current implementation makes
openClusterDetailsa required prop, which creates a breaking change for existing consumers of the ChatBot component.
🧹 Nitpick comments (1)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
117-143: Action rendering logic looks good with minor improvement opportunityThe conditional rendering of buttons based on
urlandclusterIdis well implemented. Both action types are properly handled with appropriate icons and error handling for downloads.Consider adding a fallback for actions that have neither
urlnorclusterIdto help with debugging:{message.actions.map(({ title, url, clusterId }, idx) => ( <StackItem key={idx}> {url && ( <Button onClick={() => { try { saveAs(url); } catch (error) { // eslint-disable-next-line console.error('Download failed: ', error); } }} variant="secondary" icon={<DownloadIcon />} > {title} </Button> )} {clusterId && ( <Button onClick={() => openClusterDetails(clusterId)} variant="secondary" icon={<ExternalLinkAltIcon />} > {title} </Button> )} + {!url && !clusterId && ( + console.warn('Action has no url or clusterId:', { title, url, clusterId }) + )} </StackItem> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/assisted-ui/src/components/Chatbot.tsx(3 hunks)libs/chatbot/lib/components/ChatBot/BotMessage.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBot.tsx(2 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(3 hunks)libs/chatbot/lib/components/ChatBot/helpers.ts(3 hunks)libs/chatbot/lib/hooks/use-message.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libs/chatbot/lib/hooks/use-message.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
PR: openshift-assisted/assisted-installer-ui#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/ChatBot.tsx
🧬 Code Graph Analysis (2)
libs/chatbot/lib/components/ChatBot/ChatBot.tsx (1)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
ChatBotWindowProps(26-39)
apps/assisted-ui/src/components/Chatbot.tsx (1)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
ChatBotWindowProps(26-39)
🪛 Biome (2.1.2)
libs/chatbot/lib/components/ChatBot/helpers.ts
[error] 76-83: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ 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: unit-tests
- GitHub Check: lint
- GitHub Check: circular-deps
- GitHub Check: format
🔇 Additional comments (3)
libs/chatbot/lib/components/ChatBot/ChatBot.tsx (1)
32-32: LGTM! Props forwarding implemented correctlyThe
openClusterDetailsprop is correctly forwarded to theChatBotWindowcomponent, maintaining the callback chain for cluster navigation functionality.apps/assisted-ui/src/components/Chatbot.tsx (1)
52-61: LGTM! Token handling preserved correctlyThe
onApiCallcallback correctly maintains the existing token retrieval and authorization header logic while being properly memoized to avoid unnecessary re-renders.libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
28-28: LGTM! Proper callback integrationThe
openClusterDetailsprop is correctly typed and integrated into the component props, enabling cluster navigation functionality from chat messages.
openshift-merge-bot
bot
merged commit 86daafa
into
openshift-assisted:master
|
@rawagner: new pull request created: #3113 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