Skip to content

🎨 Palette: Add confirmation dialog for chat deletion #150

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.

Sign up for GitHub

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

Draft
Dexploarer wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

🎨 Palette: Add confirmation dialog for chat deletion #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions apps/app/src/components/ConversationsSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import { useEffect, useRef, useState } from "react";
import { useApp } from "../AppContext";
import { ConfirmDeleteControl } from "./shared/confirm-delete-control";

interface ConversationsSidebarProps {
mobile?: boolean;
Expand Down Expand Up @@ -177,18 +178,30 @@ export function ConversationsSidebar({
</div>
</div>
</button>
<button
type="button"
data-testid="conv-delete"
className="opacity-100 sm:opacity-0 sm:group-hover:opacity-100 transition-opacity border-none bg-transparent text-muted hover:text-danger hover:bg-destructive-subtle cursor-pointer text-sm px-1 py-0.5 rounded flex-shrink-0"
onClick={(e) => {
e.stopPropagation();
void handleDeleteConversation(conv.id);
}}
title="Delete conversation"
{/*
Interactive wrapper for delete control.
We ignore keyboard events here because navigation and activation
are handled by the inner buttons of ConfirmDeleteControl.
*/}
{/* biome-ignore lint/a11y/useKeyWithClickEvents: interactive children handle keyboard access */}
{/* biome-ignore lint/a11y/noStaticElementInteractions: this wrapper just stops propagation */}
<div
className="flex items-center gap-1 opacity-100 sm:opacity-0 sm:group-hover:opacity-100 transition-opacity"
onClick={(e) => e.stopPropagation()}
>
×
</button>
<ConfirmDeleteControl
onConfirm={() => void handleDeleteConversation(conv.id)}
triggerLabel="×"
triggerTitle="Delete conversation"
triggerClassName="border-none bg-transparent text-muted hover:text-danger hover:bg-destructive-subtle cursor-pointer text-sm px-1 py-0.5 rounded flex-shrink-0"
promptText="Delete?"
confirmLabel="Yes"
cancelLabel="No"
confirmClassName="px-1.5 py-0.5 text-[10px] bg-danger text-white rounded cursor-pointer hover:bg-danger-hover border-none mr-1"
cancelClassName="px-1.5 py-0.5 text-[10px] bg-bg border border-border text-muted rounded cursor-pointer hover:text-txt"
promptClassName="text-[10px] text-danger mr-1"
/>
Comment on lines +192 to +203
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 26, 2026

Choose a reason for hiding this comment

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

Lack of error handling for conversation deletion

The onConfirm handler for ConfirmDeleteControl calls handleDeleteConversation(conv.id) without handling potential errors. If the deletion fails (e.g., due to a network or server error), the user receives no feedback, and the UI may become inconsistent.

Recommended solution:
Add error handling and user feedback for failed deletions. For example:

onConfirm={async () => {
  try {
    await handleDeleteConversation(conv.id);
  } catch (err) {
    // Show error notification or feedback to the user
  }
}}

Comment on lines +192 to +203
Copy link

@gemini-code-assist gemini-code-assist bot Feb 26, 2026

Choose a reason for hiding this comment

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

medium

For better performance and maintainability, these long Tailwind class strings should be extracted into constants. Defining them inside the map function causes them to be recreated for every conversation on every render.

Consider defining these constants outside the map loop, for example, at the top of the ConversationsSidebar component function:

const triggerClassName = "border-none bg-transparent text-muted hover:text-danger hover:bg-destructive-subtle cursor-pointer text-sm px-1 py-0.5 rounded flex-shrink-0";
const confirmClassName = "px-1.5 py-0.5 text-[10px] bg-danger text-white rounded cursor-pointer hover:bg-danger-hover border-none mr-1";
const cancelClassName = "px-1.5 py-0.5 text-[10px] bg-bg border border-border text-muted rounded cursor-pointer hover:text-txt";
const promptClassName = "text-[10px] text-danger mr-1";

Then use these constants in the ConfirmDeleteControl props.

</div>
</>
)}
</div>
Expand Down
3 changes: 3 additions & 0 deletions apps/app/src/components/shared/confirm-delete-control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ type ConfirmDeleteControlProps = {
onConfirm: () => void;
disabled?: boolean;
triggerLabel?: string;
triggerTitle?: string;
confirmLabel?: string;
cancelLabel?: string;
busyLabel?: string;
Expand All @@ -18,6 +19,7 @@ export function ConfirmDeleteControl({
onConfirm,
disabled = false,
triggerLabel = "Delete",
triggerTitle,
confirmLabel = "Confirm",
cancelLabel = "Cancel",
busyLabel,
Expand All @@ -36,6 +38,7 @@ export function ConfirmDeleteControl({
className={triggerClassName}
onClick={() => setConfirming(true)}
disabled={disabled}
title={triggerTitle}
>
Comment on lines +41 to 42
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 26, 2026

Choose a reason for hiding this comment

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

Accessibility Issue:
The title attribute of the trigger button is set to {triggerTitle} regardless of whether it is defined. If triggerTitle is undefined, the attribute will be set to undefined, which may cause accessibility issues or unexpected behavior in some browsers.

Recommended Solution:
Conditionally set the title attribute only if triggerTitle is defined:

{triggerTitle ? <button title={triggerTitle} ... /> : <button ... />}

Or use:

<button
  ...
  {...(triggerTitle ? { title: triggerTitle } : {})}
>

{triggerLabel}
</button>
Expand Down
84 changes: 84 additions & 0 deletions apps/app/test/app/conversations-sidebar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import React from "react";
import TestRenderer, { act } from "react-test-renderer";
import { beforeEach, describe, expect, it, vi } from "vitest";

const { mockUseApp } = vi.hoisted(() => ({
mockUseApp: vi.fn(),
}));

vi.mock("../../src/AppContext", () => ({
useApp: () => mockUseApp(),
}));

// Mock ConfirmDeleteControl to simplify testing its interaction
vi.mock("../../src/components/shared/confirm-delete-control", () => ({
ConfirmDeleteControl: ({
onConfirm,
triggerTitle,
}: { onConfirm: () => void; triggerTitle: string }) => (
<button
onClick={(e) => {
e.stopPropagation();
onConfirm();
}}
data-testid="mock-delete-btn"
title={triggerTitle}
>
Mock Delete
</button>
),
}));

import { ConversationsSidebar } from "../../src/components/ConversationsSidebar";

describe("ConversationsSidebar", () => {
const defaultContext = {
conversations: [
{ id: "c1", title: "Chat 1", updatedAt: new Date().toISOString() },
{ id: "c2", title: "Chat 2", updatedAt: new Date().toISOString() },
],
activeConversationId: "c1",
unreadConversations: new Set(),
handleNewConversation: vi.fn(),
handleSelectConversation: vi.fn(),
handleDeleteConversation: vi.fn(),
handleRenameConversation: vi.fn(),
};

beforeEach(() => {
mockUseApp.mockReturnValue(defaultContext);
});

it("renders conversation list", () => {
let tree: TestRenderer.ReactTestRenderer;
act(() => {
tree = TestRenderer.create(React.createElement(ConversationsSidebar));
});

const items = tree!.root.findAllByProps({ "data-testid": "conv-item" });
expect(items.length).toBe(2);
expect(items[0].props["data-active"]).toBe(true); // c1 is active

Check failure on line 60 in apps/app/test/app/conversations-sidebar.test.tsx

View workflow job for this annotation

GitHub Actions / Unit Tests

apps/app/test/app/conversations-sidebar.test.tsx > ConversationsSidebar > renders conversation list 

AssertionError: expected undefined to be true // Object.is equality

- Expected: 
true

+ Received: 
undefined

 ❯ apps/app/test/app/conversations-sidebar.test.tsx:60:43
});

it("uses ConfirmDeleteControl for deletion", () => {
let tree: TestRenderer.ReactTestRenderer;
act(() => {
tree = TestRenderer.create(React.createElement(ConversationsSidebar));
});

const deleteBtns = tree!.root.findAllByProps({
"data-testid": "mock-delete-btn",
});
expect(deleteBtns.length).toBe(2);

// Verify title prop is passed correctly
expect(deleteBtns[0].props.title).toBe("Delete conversation");

act(() => {
// Simulate click which triggers onConfirm in our mock
deleteBtns[0].props.onClick({ stopPropagation: vi.fn() });
});
Comment on lines +77 to +80
Copy link

@codereviewbot-ai codereviewbot-ai bot Feb 26, 2026

Choose a reason for hiding this comment

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

Direct invocation of the onClick handler with a synthetic event:

The test simulates a click by directly calling deleteBtns[0].props.onClick({ stopPropagation: vi.fn() }). This approach bypasses React's event system and may not accurately reflect user interaction, potentially leading to brittle tests if the component implementation changes.

Recommended solution:
Use a test utility that simulates user events, such as fireEvent from @testing-library/react, to trigger the click. This ensures the test more closely mirrors real user behavior and increases reliability.


expect(defaultContext.handleDeleteConversation).toHaveBeenCalledWith("c1");

Check failure on line 82 in apps/app/test/app/conversations-sidebar.test.tsx

View workflow job for this annotation

GitHub Actions / Unit Tests

apps/app/test/app/conversations-sidebar.test.tsx > ConversationsSidebar > uses ConfirmDeleteControl for deletion 

AssertionError: expected "vi.fn()" to be called with arguments: [ 'c1' ]

Received: 

  1st vi.fn() call:

  [
-   "c1",
+   "c2",
  ]


Number of calls: 1

 ❯ apps/app/test/app/conversations-sidebar.test.tsx:82:53
});
});
Loading
Loading