🐛 Refactor profile deletion dialog logic #743
Conversation
… map loop, ensuring only a single dialog instance is rendered in the DOM. The dialog's state is now managed using a 'profileToDelete' object instead of a boolean, which links the deletion action to the specific profile selected by the user. Signed-off-by: Ran Wurmbrand <[email protected]>
WalkthroughCentralizes delete confirmation handling in ProfileList.tsx by replacing per-item ConfirmDialog components with a single dialog controlled by a new state, profileToDelete: AnalysisProfile | null. Selecting Delete in a profile’s dropdown sets profileToDelete to that profile and closes the dropdown. The centralized ConfirmDialog opens when profileToDelete is non-null and references profileToDelete?.name. On confirm, it calls onDelete with profileToDelete.id and clears the state; on cancel, it clears the state. Control flow is adjusted to ensure proper dropdown closure and a single dialog lifecycle. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webview-ui/src/components/ProfileManager/ProfileList.tsx (3)
35-35: Prefer storing only the profile ID (not the whole object) to avoid stale refs and reduce re-renders.This keeps state minimal and resilient if profiles are mutated between open and confirm.
- const [profileToDelete, setProfileToDelete] = React.useState<AnalysisProfile | null>(null); + const [profileIdToDelete, setProfileIdToDelete] = React.useState<string | null>(null); + const selectedProfileName = React.useMemo( + () => profiles.find((p) => p.id === profileIdToDelete)?.name ?? "", + [profiles, profileIdToDelete] + ); @@ - onClick={() => { - setProfileToDelete(profile); - setIsOpen(false); - }} + onClick={() => { + setProfileIdToDelete(profile.id); + setIsOpen(false); + }} @@ - <ConfirmDialog - isOpen={profileToDelete !== null} + <ConfirmDialog + isOpen={profileIdToDelete !== null} title="Delete profile?" - message={`Are you sure you want to delete the profile "${profileToDelete?.name}"? This action cannot be undone.`} + message={`Are you sure you want to delete the profile "${selectedProfileName}"? This action cannot be undone.`} confirmButtonText="Delete" onConfirm={() => { - if (profileToDelete) { - onDelete(profileToDelete.id); - } - setProfileToDelete(null); + if (profileIdToDelete) { + onDelete(profileIdToDelete); + } + setProfileIdToDelete(null); }} - onCancel={() => setProfileToDelete(null)} + onCancel={() => setProfileIdToDelete(null)} />Also applies to: 126-131, 143-155
126-131: Redundant dropdown close + lint nudge.onSelect on the Dropdown already closes the menu; you can drop the extra setIsOpen(false). Also addresses the linter’s whitespace warning around this block.
- onClick={() => { - setProfileToDelete(profile); - setIsOpen(false); - }} + onClick={() => { + setProfileToDelete(profile); + }}
143-155: Use a danger-styled confirm button and forward the confirmButtonTextPass
confirmButtonVariant={ButtonVariant.danger}in ProfileList and wire theconfirmButtonTextprop through toWarningModalin ConfirmDialog (it’s currently ignored). ConfirmDialog already declaresconfirmButtonTextbut doesn’t pass it—verify thatWarningModalsupports this prop before merging.Apply in webview-ui/src/components/ProfileManager/ProfileList.tsx:
import { DataList, @@ - Icon, + Icon, + ButtonVariant, } from "@patternfly/react-core"; @@ <ConfirmDialog isOpen={profileToDelete !== null} title="Delete profile?" message={`Are you sure you want to delete the profile "${profileToDelete?.name}"? This action cannot be undone.`} - confirmButtonText="Delete" + confirmButtonText="Delete" + confirmButtonVariant={ButtonVariant.danger} onConfirm={() => { if (profileToDelete) { onDelete(profileToDelete.id); setProfileToDelete(null); }} onCancel={() => setProfileToDelete(null)} />Apply in webview-ui/src/components/ConfirmDialog/ConfirmDialog.tsx:
return ( - <WarningModal + <WarningModal isOpen={isOpen} title={title} + confirmButtonText={confirmButtonText} confirmButtonVariant={confirmButtonVariant} onClose={onCancel} onConfirm={onConfirm} > {message} </WarningModal>
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
webview-ui/src/components/ProfileManager/ProfileList.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webview-ui/src/components/ProfileManager/ProfileList.tsx (2)
shared/src/types/types.ts (1)
AnalysisProfile(291-298)webview-ui/src/components/ConfirmDialog/ConfirmDialog.tsx (1)
ConfirmDialog(6-34)
🪛 GitHub Check: Lint
webview-ui/src/components/ProfileManager/ProfileList.tsx
[warning] 126-126:
Insert ·
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
1 participant
Fixes #565
The changes address an unintended side effect where multiple dialog instances were being rendered, leading to incorrect profile deletion. The fix consists of two parts:
Centralized Dialog Rendering: The
ConfirmDialogcomponent was moved outside theprofiles.map()loop. This change ensures only one dialog instance is ever present in the DOM, preventing the rendering of multiple hidden dialogs.Explicit State Management: The state variable for managing the dialog's visibility was updated from a simple boolean to a
profileToDeleteobject. This change provides a direct and explicit link between the dialog and the specific profile selected for deletion, ensuring the correct profile is targeted every time.Summary by CodeRabbit