MGMT-21647: Add delete conversation option in chat history#3150
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rawagner: This pull request references MGMT-21647 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. |
WalkthroughAdds per-conversation deletion to ChatBot history with a confirmation modal that calls DELETE /v1/conversations/{id}, refreshes the conversation list (via fetchConversations(signal) which supports AbortSignal and sorts by created_at desc), and optionally starts a new conversation if the active one was deleted. Updates startNewConversation to accept an optional closeDrawer flag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant History as ChatBotHistory
participant Modal as DeleteConversationModal
participant API as Backend API
participant Window as ChatBotWindow
User->>History: Click delete on conversation row
History->>Modal: Open with conversation & onDelete
User->>Modal: Confirm delete
Modal->>API: DELETE /v1/conversations/{id}
alt Success
API-->>Modal: 200/204
Modal->>History: Resolve onDelete
History->>History: If deleted active → startNewConversation(false)
History->>API: GET /v1/conversations (fetchConversations with signal)
API-->>History: Sorted list
History-->>User: Updated list or empty state
else Error
API-->>Modal: Error payload
Modal-->>User: Show inline error (parsed)
end
note over Window,History: startNewConversation(closeDrawer?) propagated — Window only closes drawer if closeDrawer ≠ false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: This pull request references MGMT-21647 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: This pull request references MGMT-21647 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: 2
🧹 Nitpick comments (7)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (2)
45-45: Fix OUIA ID typo.Typo in ouiaId breaks stable selectors.
Apply:
- ouiaId="DeleteConversaionModal" + ouiaId="DeleteConversationModal"
62-64: Optional: Add test-friendly selector to error text.If you assert on the error body in tests, consider a data-attr on the Alert body.
- <Alert isInline variant="danger" title="Failed to delete conversation"> + <Alert isInline variant="danger" title="Failed to delete conversation" data-testid="delete-conversation-error"> {error} </Alert>libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
108-111: Wrap startNewConversation in useCallback to keep a stable prop.Prevents re-renders of ChatBotHistory caused by a new function identity each render.
Apply:
- startNewConversation={(closeDrawer = true) => { - startNewConversation(); - closeDrawer && setIsDrawerOpen(false); - }} + startNewConversation={handleStartNewConversation}And add above the return:
const handleStartNewConversation = React.useCallback((closeDrawer: boolean = true) => { startNewConversation(); if (closeDrawer) setIsDrawerOpen(false); }, [startNewConversation]);libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx (4)
86-88: Use functional state update when toggling drawer.Avoids stale-closure bugs if multiple rapid toggles occur.
- onDrawerToggle={() => { - setIsOpen(!isOpen); - }} + onDrawerToggle={() => { + setIsOpen((prev) => !prev); + }}- itemId !== undefined && void loadConversation(`${itemId}`); - setIsOpen(!isOpen); + itemId !== undefined && void loadConversation(`${itemId}`); + setIsOpen((prev) => !prev);Also applies to: 105-107
90-102: Improve action a11y label.Make the action’s label descriptive for screen readers.
- <MenuItemAction + <MenuItemAction icon={<TrashAltIcon />} actionId="delete" onClick={() => setDeleteConversation(c.id)} - aria-label="delete" + aria-label={`Delete conversation ${c.id}`} />
153-156: Fix comment typo.Minor typo in the inline comment.
- // if current conversaiontId is deleted, start new conversation + // if current conversationId is deleted, start a new conversation
37-54: Consider stable date formatting.toLocaleString() varies by locale/timezone; if tests or UX expect consistency, consider an explicit formatter (e.g., Intl.DateTimeFormat with a fixed locale/timeZone) or a relative time label.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(1 hunks)libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (1)
libs/ui-lib-tests/cypress/views/reusableComponents/Alert.ts (1)
Alert(1-20)
⏰ 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: translation-files
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (3)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (1)
28-39: Solid delete flow + disable-close during action.State handling, error surfacing, and preventing close while deleting look good.
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx (2)
129-160: End-to-end flow looks good.Modal wiring, active-conversation handling, and refresh after delete are coherent.
133-151: Verify DELETE /v1/conversations/{id} response contract
I didn’t find any spec or handler in the repo that indicates whether this endpoint returns HTTP 204 or a JSON payload on success (and what the exact error schema is). Please confirm the API contract and update the DELETE response parsing accordingly.
rawagner
force-pushed
the
history_delete
branch
from
f7fd565 to
45e0a18
Compare
|
@rawagner: This pull request references MGMT-21647 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
history_delete
branch
from
45e0a18 to
5f5c977
Compare
|
@rawagner: This pull request references MGMT-21647 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
history_delete
branch
from
5f5c977 to
10ded21
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx (1)
133-151: Fix DELETE handling: avoid double-reading body and handle 204/non-JSON responsesCurrent code calls
resp.json()twice and assumes JSON on success. This will throw on 204 No Content or non-JSON bodies.onDelete={async () => { const resp = await onApiCall(`/v1/conversations/${deleteConversation}`, { method: 'DELETE', }); - if (!resp.ok) { - let errMsg = `Unexpected response code ${resp.status}`; - try { - const errDetail = (await resp.json()) as { detail?: { cause?: string } }; - if (errDetail.detail?.cause) { - errMsg = errDetail.detail.cause; - } - } catch { - //failed to get err cause - } - throw errMsg; - } - const deleteResult = (await resp.json()) as { success: boolean; response: string }; - if (!deleteResult.success) { - throw deleteResult.response; - } - - // if current conversaiontId is deleted, start new conversation + const contentType = resp.headers.get('content-type') || ''; + if (!resp.ok) { + let errMsg = `Unexpected response code ${resp.status}`; + if (contentType.includes('application/json')) { + try { + const errDetail = (await resp.json()) as { detail?: { cause?: string } }; + errMsg = errDetail.detail?.cause || errMsg; + } catch { + // ignore JSON parse failure + } + } else { + try { + const text = await resp.text(); + if (text) errMsg = text; + } catch { + // ignore body read failure + } + } + throw errMsg; + } + // Success: handle JSON or 204/empty body + if (resp.status !== 204 && contentType.includes('application/json')) { + const deleteResult = (await resp.json()) as { success?: boolean; response?: string }; + if (deleteResult?.success === false) { + throw deleteResult.response || 'Delete failed'; + } + } + + // If the current conversationId is deleted, start a new conversation (keep drawer open) if (deleteConversation === conversationId) { startNewConversation(false); } await fetchConversations(); }}Also applies to: 153-159
🧹 Nitpick comments (3)
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx (3)
37-54: Make list building defensive (null-safe) and avoid in-place sortUse a copied array to avoid accidental mutation and guard for missing
conversations.- const cnvs = (await resp.json()) as ConversationHistory; - setConversations( - cnvs.conversations - .sort((a, b) => Date.parse(b.created_at) - Date.parse(a.created_at)) - .map(({ conversation_id, created_at }) => ({ - id: conversation_id, - text: new Date(created_at).toLocaleString(), - })), - ); + const cnvs = (await resp.json()) as ConversationHistory; + const list = (cnvs.conversations || []) + .slice() + .sort((a, b) => Date.parse(b.created_at) - Date.parse(a.created_at)) + .map(({ conversation_id, created_at }) => ({ + id: conversation_id, + text: new Date(created_at).toLocaleString(), + })); + setConversations(list);
86-107: Close/toggle drawer deterministically; improve a11y label
- Use functional state update for toggle to avoid stale captures.
- When selecting an item, set to closed explicitly.
- Provide a clearer aria-label on the delete action.
onDrawerToggle={() => { - setIsOpen(!isOpen); + setIsOpen((open) => !open); }} @@ - <MenuItemAction + <MenuItemAction icon={<TrashAltIcon />} actionId="delete" onClick={() => setDeleteConversation(c.id)} - aria-label="delete" + aria-label="Delete conversation" /> @@ - onSelectActiveItem={(_, itemId) => { - itemId !== undefined && void loadConversation(`${itemId}`); - setIsOpen(!isOpen); - }} + onSelectActiveItem={(_, itemId) => { + if (itemId !== undefined) { + void loadConversation(`${itemId}`); + } + setIsOpen(false); + }}Also applies to: 93-101
153-156: Typo in comment: “conversaiontId” → “conversationId”Non-user-facing, but easy win for readability.
- // if current conversaiontId is deleted, start new conversation + // If the current conversationId is deleted, start a new conversation (keep drawer open)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(1 hunks)libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/chatbot/lib/components/ChatBot/DeleteConversationModal.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: translation-files
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: tests
🔇 Additional comments (3)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
108-111: Wrapper preserves behavior; good changeThe optional
closeDrawerflag keeps existing UX by default while enabling history-driven flows to keep the drawer open. Looks correct.libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx (2)
7-11: Imports align with new UI affordances
Alert,MenuItemAction,TrashAltIcon, andDeleteConversationModalare appropriate for the delete flow.
18-21: Signature expansion verified – all call sites updated
Only ChatBotWindow defines the optionalcloseDrawerparameter and ChatBotHistory invokes it withfalse. No remaining callers rely on a zero-arg signature.
|
/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. |
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
history_delete
branch
from
10ded21 to
6423763
Compare
|
@rawagner: This pull request references MGMT-21647 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.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (1)
47-56: ARIA labelling is now aligned.
aria-labelledbymatchesModalHeader’slabelId. Good fix.
🧹 Nitpick comments (4)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (4)
29-40: Avoid setState after unmount in success path.
onClose()may unmount the modal beforefinallyruns, causing a state update on an unmounted component. SetisDeletingto false before closing.const handleDelete = React.useCallback(async () => { setError(undefined); setIsDeleting(true); try { - await onDelete(); - onClose(); + await onDelete(); } catch (e) { setError(getErrorMessage(e)); - } finally { - setIsDeleting(false); - } + return setIsDeleting(false); + } + setIsDeleting(false); + onClose(); }, [onDelete, onClose]);
60-61: Tighten user-facing copy; avoid dumping long/opaque content.Use a neutral prompt; optionally show a short, meaningful detail elsewhere (e.g., date). Long
conversation.textcan overflow.- Are you sure you want to delete conversation from <b>{conversation.text}</b>? + Are you sure you want to delete this conversation?
64-66: Announce errors to screen readers.Make the inline alert a live region.
- <Alert isInline variant="danger" title="Failed to delete conversation"> + <Alert isInline variant="danger" title="Failed to delete conversation" isLiveRegion> {error} </Alert>
72-83: Drop unnecessary keys and set explicit button type.
keyis irrelevant here; addtype="button"to avoid unintended form submits if embedded in a form.- <Button - key="confirm" + <Button + type="button" variant="danger" onClick={() => void handleDelete()} isDisabled={isDeleting} isLoading={isDeleting} > Delete </Button> - <Button key="cancel" variant="link" onClick={onClose} isDisabled={isDeleting}> + <Button type="button" variant="link" onClick={onClose} isDisabled={isDeleting}> Cancel </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(1 hunks)libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/chatbot/lib/components/ChatBot/ChatBotHistory.tsx
- libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/chatbot/lib/components/ChatBot/DeleteConversationModal.tsx (1)
libs/ui-lib-tests/cypress/views/reusableComponents/Alert.ts (1)
Alert(1-20)
⏰ 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: translation-files
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: lint
|
[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 |
openshift-merge-bot
bot
merged commit 8ea9486
into
openshift-assisted:master
|
@rawagner: new pull request created: #3157 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
Improvements