Skip to content

fix: moved the conversation delete button into conversation settings #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 19, 2025

Conversation

ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented May 19, 2025

Important

Move conversation delete button from ConversationList to ConversationSettings and adjust related state management.

  • UI Changes:
    • Move delete button from ConversationList to ConversationSettings.
    • Add "Danger Zone" section in ConversationSettings for delete action.
  • State Management:
    • Remove deleteDialogOpen and conversationToDelete state from ConversationList.
    • Add deleteDialogOpen state to ConversationSettings.
  • Imports:
    • Remove Trash and DeleteConversationConfirmationDialog imports from ConversationList.tsx.
    • Add Trash and DeleteConversationConfirmationDialog imports to ConversationSettings.tsx.

This description was created by Ellipsis for 6ee1097. You can customize this summary. It will automatically update as commits are pushed.

@ErikBjare ErikBjare merged commit 39ad54e into master May 19, 2025
1 check passed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 6ee1097 in 1 minute and 59 seconds. Click for details.
  • Reviewed 144 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/components/ConversationList.tsx:1
  • Draft comment:
    Removed the Trash import and deletion-related state & UI from ConversationList. Confirm that deletion is now exclusively handled in ConversationSettings as intended.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. src/components/ConversationSettings.tsx:1001
  • Draft comment:
    The DeleteConversationConfirmationDialog is passed 'conversationId' via its 'conversationName' prop. Verify that this naming is intentional, or consider renaming the prop for clarity if conversationId isn’t actually a human‐readable name.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment identifies a real naming inconsistency, it's phrased as a request to "verify" intention rather than making a clear suggestion. The comment also uses speculative language ("if conversationId isn't actually..."). Per the rules, we should not ask authors to verify things or make speculative comments. A better comment would directly suggest renaming the prop to match its usage. The naming inconsistency could be a deliberate choice - maybe the ID is actually used as a display name in the dialog. Without seeing the DeleteConversationConfirmationDialog implementation, we can't be certain. Even with that uncertainty, the code would be clearer if the prop name matched its usage. If the ID is used as a display name, that transformation should happen inside the dialog component, not through confusing prop naming. While the comment identifies a real issue, its phrasing as a verification request violates the commenting rules. The comment should be removed.
3. src/components/ConversationSettings.tsx:1000
  • Draft comment:
    Consider whether rendering the DeleteConversationConfirmationDialog inside the element could lead to unintended interactions with form submission. It might be beneficial to render the modal outside the form context if not required.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The dialog is used for deletion which is separate from the form's purpose of updating settings. While technically it could stay inside the form without breaking anything (since the delete button uses onClick handlers rather than form submission), it's cleaner architecturally to separate concerns. However, this is a relatively minor architectural suggestion rather than a critical issue. The comment may be overly cautious - there's no clear evidence of actual problems with the current implementation. The dialog uses its own handlers and doesn't interfere with form submission. While the current implementation works fine, separating the deletion dialog from the settings form would be a cleaner architecture that better separates concerns. This is a valid suggestion for improving code organization, but not critical enough to warrant keeping the comment since it doesn't highlight any actual problems with the current implementation.

Workflow ID: wflow_RF5CQC6XbloygoOr

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@@ -947,7 +950,32 @@ export const ConversationSettings: FC<ConversationSettingsProps> = ({ conversati
</CollapsibleContent>
</FormItem>
</Collapsible>

{/* Danger Zone */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the Danger Zone section with a delete button that opens a confirmation dialog. Note that the onDelete callback only performs a redirect (window.location.href = '/') without updating the local conversation store. Consider also updating application state or handling API deletion responses to keep the UI consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant