Skip to content

Commit

Permalink
Improve conversation panel (#6087)
Browse files Browse the repository at this point in the history
  • Loading branch information
amanape authored Jan 7, 2025
1 parent 77aa843 commit cf0f6e5
Show file tree
Hide file tree
Showing 19 changed files with 162 additions and 146 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { afterEach, describe, expect, it, test, vi } from "vitest";
import userEvent from "@testing-library/user-event";
import { formatTimeDelta } from "#/utils/format-time-delta";
import { ConversationCard } from "#/components/features/conversation-panel/conversation-card";
import { clickOnEditButton } from "./utils";

describe("ConversationCard", () => {
const onClick = vi.fn();
Expand Down Expand Up @@ -144,7 +145,9 @@ describe("ConversationCard", () => {
/>,
);

const selectedRepository = screen.getByTestId("conversation-card-selected-repository");
const selectedRepository = screen.getByTestId(
"conversation-card-selected-repository",
);
await user.click(selectedRepository);

expect(onClick).not.toHaveBeenCalled();
Expand All @@ -164,13 +167,22 @@ describe("ConversationCard", () => {
);

const title = screen.getByTestId("conversation-card-title");
expect(title).toBeDisabled();

await clickOnEditButton(user);

expect(title).toBeEnabled();
expect(screen.queryByTestId("context-menu")).not.toBeInTheDocument();
// expect to be focused
expect(document.activeElement).toBe(title);

await user.clear(title);
await user.type(title, "New Conversation Name ");
await user.tab();

expect(onChangeTitle).toHaveBeenCalledWith("New Conversation Name");
expect(title).toHaveValue("New Conversation Name");
expect(title).toBeDisabled();
});

it("should reset title and not call onChangeTitle when the title is empty", async () => {
Expand All @@ -186,6 +198,8 @@ describe("ConversationCard", () => {
/>,
);

await clickOnEditButton(user);

const title = screen.getByTestId("conversation-card-title");

await user.clear(title);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import userEvent from "@testing-library/user-event";
import { ConversationPanel } from "#/components/features/conversation-panel/conversation-panel";
import OpenHands from "#/api/open-hands";
import { AuthProvider } from "#/context/auth-context";
import { clickOnEditButton } from "./utils";

describe("ConversationPanel", () => {
const onCloseMock = vi.fn();
Expand Down Expand Up @@ -52,6 +53,8 @@ describe("ConversationPanel", () => {
renderConversationPanel();
const cards = await screen.findAllByTestId("conversation-card");

// NOTE that we filter out conversations that don't have a created_at property
// (mock data has 4 conversations, but only 3 have a created_at property)
expect(cards).toHaveLength(3);
});

Expand Down Expand Up @@ -169,6 +172,8 @@ describe("ConversationPanel", () => {
const cards = await screen.findAllByTestId("conversation-card");
const title = within(cards[0]).getByTestId("conversation-card-title");

await clickOnEditButton(user);

await user.clear(title);
await user.type(title, "Conversation 1 Renamed");
await user.tab();
Expand Down Expand Up @@ -196,6 +201,8 @@ describe("ConversationPanel", () => {
// Ensure the conversation is not renamed
expect(updateUserConversationSpy).not.toHaveBeenCalled();

await clickOnEditButton(user);

await user.type(title, "Conversation 1");
await user.click(title);
await user.tab();
Expand All @@ -217,51 +224,4 @@ describe("ConversationPanel", () => {

expect(onCloseMock).toHaveBeenCalledOnce();
});

describe("New Conversation Button", () => {
it("should display a confirmation modal when clicking", async () => {
const user = userEvent.setup();
renderConversationPanel();

expect(
screen.queryByTestId("confirm-new-conversation-modal"),
).not.toBeInTheDocument();

const newProjectButton = screen.getByTestId("new-conversation-button");
await user.click(newProjectButton);

const modal = screen.getByTestId("confirm-new-conversation-modal");
expect(modal).toBeInTheDocument();
});

it("should call endSession and close panel after confirming", async () => {
const user = userEvent.setup();
renderConversationPanel();

const newProjectButton = screen.getByTestId("new-conversation-button");
await user.click(newProjectButton);

const confirmButton = screen.getByText("Confirm");
await user.click(confirmButton);

expect(endSessionMock).toHaveBeenCalledOnce();
expect(onCloseMock).toHaveBeenCalledOnce();
});

it("should close the modal when cancelling", async () => {
const user = userEvent.setup();
renderConversationPanel();

const newProjectButton = screen.getByTestId("new-conversation-button");
await user.click(newProjectButton);

const cancelButton = screen.getByText("Cancel");
await user.click(cancelButton);

expect(endSessionMock).not.toHaveBeenCalled();
expect(
screen.queryByTestId("confirm-new-conversation-modal"),
).not.toBeInTheDocument();
});
});
});
12 changes: 12 additions & 0 deletions frontend/__tests__/components/features/conversation-panel/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { screen, within } from "@testing-library/react";
import { UserEvent } from "@testing-library/user-event";

export const clickOnEditButton = async (user: UserEvent) => {
const ellipsisButton = screen.getByTestId("ellipsis-button");
await user.click(ellipsisButton);

const menu = screen.getByTestId("context-menu");
const editButton = within(menu).getByTestId("edit-button");

await user.click(editButton);
};
1 change: 1 addition & 0 deletions frontend/__tests__/routes/_oh.app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ describe("App", () => {
getConversationSpy.mockResolvedValue({
conversation_id: "9999",
last_updated_at: "",
created_at: "",
title: "",
selected_repository: "",
status: "STOPPED",
Expand Down
1 change: 1 addition & 0 deletions frontend/src/api/open-hands.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export interface Conversation {
title: string;
selected_repository: string | null;
last_updated_at: string;
created_at: string;
status: ProjectStatus;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import { ContextMenuListItem } from "../context-menu/context-menu-list-item";
interface ConversationCardContextMenuProps {
onClose: () => void;
onDelete: (event: React.MouseEvent<HTMLButtonElement>) => void;
onEdit: (event: React.MouseEvent<HTMLButtonElement>) => void;
}

export function ConversationCardContextMenu({
onClose,
onDelete,
onEdit,
}: ConversationCardContextMenuProps) {
const ref = useClickOutsideElement<HTMLUListElement>(onClose);

Expand All @@ -22,6 +24,9 @@ export function ConversationCardContextMenu({
<ContextMenuListItem testId="delete-button" onClick={onDelete}>
Delete
</ContextMenuListItem>
<ContextMenuListItem testId="edit-button" onClick={onEdit}>
Edit Title
</ContextMenuListItem>
</ContextMenu>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export function ConversationCard({
status = "STOPPED",
}: ConversationCardProps) {
const [contextMenuVisible, setContextMenuVisible] = React.useState(false);
const [titleMode, setTitleMode] = React.useState<"view" | "edit">("view");
const inputRef = React.useRef<HTMLInputElement>(null);

const handleBlur = () => {
Expand All @@ -39,6 +40,8 @@ export function ConversationCard({
// reset the value if it's empty
inputRef.current!.value = title;
}

setTitleMode("view");
};

const handleKeyUp = (event: React.KeyboardEvent<HTMLInputElement>) => {
Expand All @@ -56,6 +59,18 @@ export function ConversationCard({
onDelete();
};

const handleEdit = (event: React.MouseEvent<HTMLButtonElement>) => {
event.stopPropagation();
setTitleMode("edit");
setContextMenuVisible(false);
};

React.useEffect(() => {
if (titleMode === "edit") {
inputRef.current?.focus();
}
}, [titleMode]);

return (
<div
data-testid="conversation-card"
Expand All @@ -64,8 +79,9 @@ export function ConversationCard({
>
<div className="flex items-center justify-between space-x-1">
<input
ref={inputRef}
data-testid="conversation-card-title"
ref={inputRef}
disabled={titleMode === "view"}
onClick={handleInputClick}
onBlur={handleBlur}
onKeyUp={handleKeyUp}
Expand All @@ -88,6 +104,7 @@ export function ConversationCard({
<ConversationCardContextMenu
onClose={() => setContextMenuVisible(false)}
onDelete={handleDelete}
onEdit={handleEdit}
/>
)}
{selectedRepository && (
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import ReactDOM from "react-dom";

interface ConversationPanelWrapperProps {
isOpen: boolean;
}

export function ConversationPanelWrapper({
isOpen,
children,
}: React.PropsWithChildren<ConversationPanelWrapperProps>) {
if (!isOpen) return null;

const portalTarget = document.getElementById("root-outlet");
if (!portalTarget) return null;

return ReactDOM.createPortal(
<div className="absolute h-full w-full left-0 top-0 z-20 bg-black/80 rounded-xl">
{children}
</div>,
portalTarget,
);
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import React from "react";
import { useLocation, useNavigate, useParams } from "react-router";
import { useNavigate, useParams } from "react-router";
import { ConversationCard } from "./conversation-card";
import { useUserConversations } from "#/hooks/query/use-user-conversations";
import { useDeleteConversation } from "#/hooks/mutation/use-delete-conversation";
import { ConfirmDeleteModal } from "./confirm-delete-modal";
import { NewConversationButton } from "./new-conversation-button";
import { LoadingSpinner } from "#/components/shared/loading-spinner";
import { useUpdateConversation } from "#/hooks/mutation/use-update-conversation";
import { useEndSession } from "#/hooks/use-end-session";
import { ExitConversationModal } from "./exit-conversation-modal";
import { useClickOutsideElement } from "#/hooks/use-click-outside-element";

interface ConversationPanelProps {
onClose: () => void;
Expand All @@ -17,9 +17,8 @@ interface ConversationPanelProps {
export function ConversationPanel({ onClose }: ConversationPanelProps) {
const { conversationId: cid } = useParams();
const navigate = useNavigate();
const location = useLocation();

const endSession = useEndSession();
const ref = useClickOutsideElement<HTMLDivElement>(onClose);

const [confirmDeleteModalVisible, setConfirmDeleteModalVisible] =
React.useState(false);
Expand Down Expand Up @@ -71,15 +70,11 @@ export function ConversationPanel({ onClose }: ConversationPanelProps) {

return (
<div
ref={ref}
data-testid="conversation-panel"
className="w-[350px] h-full border border-neutral-700 bg-neutral-800 rounded-xl overflow-y-auto"
>
<div className="pt-4 px-4 flex items-center justify-between">
{location.pathname.startsWith("/conversation") && (
<NewConversationButton
onClick={() => setConfirmExitConversationModalVisible(true)}
/>
)}
{isFetching && <LoadingSpinner size="small" />}
</div>
{error && (
Expand Down
Loading

0 comments on commit cf0f6e5

Please sign in to comment.