Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions apps/app/src/components/sidebar/ProjectRow.interactions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,18 @@ vi.mock("@/components/project/ProjectActionsProvider", () => ({
}),
}));

vi.mock("@/components/thread/ThreadActionsProvider", () => ({
useThreadActions: () => ({
renameThread: vi.fn(),
requestRename: vi.fn(),
requestDelete: vi.fn(),
archiveThreadAndChildren: vi.fn(),
unarchiveThread: vi.fn(),
togglePin: vi.fn(),
toggleRead: vi.fn(),
}),
}));

function makeProject(): ProjectResponse {
return {
id: "proj_test",
Expand Down
67 changes: 65 additions & 2 deletions apps/app/src/components/sidebar/ThreadRow.test.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,27 @@
// @vitest-environment jsdom

import { act, cleanup, render, screen } from "@testing-library/react";
import { act, cleanup, fireEvent, render, screen } from "@testing-library/react";
import { MemoryRouter } from "react-router-dom";
import type { ReactNode } from "react";
import { createStore, Provider } from "jotai";
import type { ThreadListEntry } from "@bb/domain";
import type { PluginComposerThreadRowStatus } from "@bb/plugin-sdk";
import { afterEach, describe, expect, it, vi } from "vitest";
import { ThreadRow, type ThreadRowOptions } from "./ThreadRow";
import {
resetSidebarTitleDoubleClickForTest,
ThreadRow,
type ThreadRowOptions,
} from "./ThreadRow";

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

vi.mock("@/components/thread/ThreadActionsProvider", () => ({
useThreadActions: () => ({
renameThread: mocks.renameThread,
}),
}));
import { SidebarThreadTitleMentionResourcesProvider } from "./SidebarThreadTitleMentions";
import {
SIDEBAR_ROW_OPEN_IN_SPLIT_STATE_CLASS,
Expand Down Expand Up @@ -225,6 +239,8 @@ function renderSplitThreadRow({

afterEach(() => {
cleanup();
mocks.renameThread.mockReset();
resetSidebarTitleDoubleClickForTest();
resetPluginThreadRowStatusesForTest();
// The layout is tab-scoped, so it lands in both stores (createTabScopedStorage).
window.localStorage.removeItem(SPLIT_LAYOUT_STORAGE_KEY);
Expand Down Expand Up @@ -1257,4 +1273,51 @@ describe("ThreadRow", () => {
expect(container.querySelector('[data-icon="CircleCheck"]')).toBeNull();
expect(screen.getByLabelText("Unread thread succeeded")).not.toBeNull();
});

it("edits the row title inline after a double click and commits on Enter", () => {
renderThreadRow({
thread: createThread({ title: "Thread", titleFallback: "Thread" }),
});

fireEvent.doubleClick(screen.getByText("Thread"));
const input = screen.getByRole("textbox", { name: "Thread name" });
expect(input).toHaveProperty("value", "Thread");

fireEvent.change(input, { target: { value: "Renamed thread" } });
fireEvent.keyDown(input, { key: "Enter" });

expect(mocks.renameThread).toHaveBeenCalledWith("thr_test", "Renamed thread");
expect(screen.queryByRole("textbox", { name: "Thread name" })).toBeNull();
expect(screen.getByText("Thread")).not.toBeNull();
});

it("cancels an inline row rename on Escape without saving", () => {
renderThreadRow({
thread: createThread({ title: "Thread", titleFallback: "Thread" }),
});

fireEvent.doubleClick(screen.getByText("Thread"));
const input = screen.getByRole("textbox", { name: "Thread name" });
fireEvent.change(input, { target: { value: "Scratch name" } });
fireEvent.keyDown(input, { key: "Escape" });

expect(mocks.renameThread).not.toHaveBeenCalled();
expect(screen.queryByRole("textbox", { name: "Thread name" })).toBeNull();
expect(screen.getByText("Thread")).not.toBeNull();
});

it("starts a rename from a second click after the row remounts", () => {
const thread = createThread({ title: "Thread", titleFallback: "Thread" });
const { rerenderThreadRow } = renderThreadRow({ thread });
const link = screen.getByRole("link", { name: "Open Thread" });

fireEvent.click(link);
rerenderThreadRow(thread);
fireEvent.click(screen.getByRole("link", { name: "Open Thread" }));

expect(screen.getByRole("textbox", { name: "Thread name" })).toHaveProperty(
"value",
"Thread",
);
});
});
72 changes: 69 additions & 3 deletions apps/app/src/components/sidebar/ThreadRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
ThreadActionsContextMenu,
ThreadActionsMenu,
} from "@/components/thread/ThreadActionsMenu";
import { useThreadActions } from "@/components/thread/ThreadActionsProvider";
import { useInlineThreadTitle } from "@/components/thread/InlineThreadTitle";
import {
COARSE_POINTER_COMPACT_ROW_HEIGHT_CLASS,
COARSE_POINTER_GLYPH_BOX_CLASS,
Expand Down Expand Up @@ -75,6 +77,25 @@ import { useThreadTitleDisplayText } from "@/components/thread/ThreadTitleMentio
import { pluginIconName } from "@/components/plugin/PluginIcon";
import { usePluginThreadRowStatus } from "@/lib/plugin-thread-row-status";

const SIDEBAR_TITLE_DOUBLE_CLICK_MS = 400;

let lastSidebarTitleClick: { at: number; threadId: string } | null = null;

function consumeSidebarTitleDoubleClick(threadId: string): boolean {
const now = Date.now();
const previous = lastSidebarTitleClick;
lastSidebarTitleClick = { at: now, threadId };
return (
previous !== null &&
previous.threadId === threadId &&
now - previous.at < SIDEBAR_TITLE_DOUBLE_CLICK_MS
);
}

export function resetSidebarTitleDoubleClickForTest(): void {
lastSidebarTitleClick = null;
}

interface ThreadRowBaseOptions {
depth: number;
isCompact: boolean;
Expand Down Expand Up @@ -480,6 +501,7 @@ function ThreadRowComponent({
}: ThreadRowProps) {
const [isDropdownActionsOpen, setIsDropdownActionsOpen] = useState(false);
const [isContextActionsOpen, setIsContextActionsOpen] = useState(false);
const { renameThread } = useThreadActions();
const setConversationCollapsed = useSetAtom(
getThreadConversationCollapsedAtom(thread.id),
);
Expand All @@ -501,6 +523,25 @@ function ThreadRowComponent({
// Inside a section the row shows the leaf but keeps the full path for a11y.
const visibleTitle = displayTitle ?? threadTitle;
const labelTitle = useThreadTitleDisplayText(accessibleTitle ?? threadTitle);
const handleRename = useCallback(
(nextTitle: string) => {
renameThread(thread.id, nextTitle);
},
[renameThread, thread.id],
);
const { editor, isEditing, startEditing } = useInlineThreadTitle({
onCommit: handleRename,
resetKey: thread.id,
title: threadTitle,
});
const startTitleEditing = useCallback(
(event: { preventDefault: () => void; stopPropagation: () => void }) => {
event.preventDefault();
event.stopPropagation();
startEditing();
},
[startEditing],
);
const threadSplitsEnabled = useThreadSplitsEnabled();
const splitIndicator = usePaneContentSplitIndicator(
{ kind: "thread", projectId, threadId: thread.id },
Expand Down Expand Up @@ -625,6 +666,11 @@ function ThreadRowComponent({
data-sidebar-thread-shortcut-target=""
data-sidebar-thread-id={thread.id}
onClick={(event) => {
if (isEditing) {
event.preventDefault();
event.stopPropagation();
return;
}
// Selecting a thread/agent row restores its conversation without
// disturbing any other thread's collapsed conversation state.
setConversationCollapsed(false);
Expand All @@ -636,16 +682,36 @@ function ThreadRowComponent({
openInSplit();
return;
}
// A first click may navigate and remount this row. Remember that
// click so the second click of a double-click can still open the
// editor after the remount.
if (consumeSidebarTitleDoubleClick(thread.id)) {
event.preventDefault();
event.stopPropagation();
startEditing();
return;
}
onProjectSelect?.();
}}
onDoubleClick={isEditing ? undefined : startTitleEditing}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🚨 slopcop/review — The sidebar double-click does not open the editor.

A browser sends two click events before dblclick. The first click follows this link and removes the row. Therefore, this handler never starts the editor. I reproduced this behavior in Chromium against the development app. Please keep the first click on the current route until the double-click decision completes. Please add a browser-level test, or a test that sends the real click sequence.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. A module-level click mark now survives a remount so the second click still opens the editor.

aria-label={linkLabel}
aria-keyshortcuts={shortcut?.ariaKeyshortcuts}
className="absolute inset-0 rounded-md outline-none ring-sidebar-ring focus-visible:ring-2"
/>
<span className="flex min-w-0 flex-1 items-center gap-1.5">
<span className="min-w-0 truncate" title={labelTitle}>
<SidebarThreadTitle title={visibleTitle} />
</span>
{isEditing ? (
<span className="relative z-10 min-w-0 flex-1 overflow-visible">
{editor}
</span>
) : (
<span
className="min-w-0 truncate"
title={labelTitle}
onDoubleClick={startTitleEditing}
>
<SidebarThreadTitle title={visibleTitle} />
</span>
)}
{parentOptions && hasChildren ? (
<SidebarChildToggleChevron
isCollapsed={isParentCollapsed}
Expand Down
121 changes: 121 additions & 0 deletions apps/app/src/components/thread/InlineThreadTitle.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// @vitest-environment jsdom

import { cleanup, fireEvent, render, screen } from "@testing-library/react";
import { afterEach, describe, expect, it, vi } from "vitest";
import {
resolveInlineThreadTitleCommit,
useInlineThreadTitle,
} from "./InlineThreadTitle";

afterEach(() => {
cleanup();
});

function InlineTitleHarness({
onCommit,
resetKey = "thr_test",
title,
}: {
onCommit: (nextTitle: string) => void;
resetKey?: string;
title: string;
}) {
const { editor, isEditing, startEditing } = useInlineThreadTitle({
onCommit,
resetKey,
title,
});

return (
<div>
{isEditing ? (
editor
) : (
<button type="button" onDoubleClick={startEditing}>
{title}
</button>
)}
</div>
);
}

describe("resolveInlineThreadTitleCommit", () => {
it("commits a trimmed new title", () => {
expect(
resolveInlineThreadTitleCommit({
currentTitle: "Old name",
nextTitle: " New name ",
}),
).toEqual({ kind: "commit", title: "New name" });
});

it("cancels an empty or unchanged title", () => {
expect(
resolveInlineThreadTitleCommit({
currentTitle: "Same name",
nextTitle: " ",
}),
).toEqual({ kind: "cancel" });
expect(
resolveInlineThreadTitleCommit({
currentTitle: "Same name",
nextTitle: " Same name ",
}),
).toEqual({ kind: "cancel" });
});
});

describe("useInlineThreadTitle", () => {
it("commits a changed title on blur and ignores a second close", () => {
const onCommit = vi.fn();
render(<InlineTitleHarness onCommit={onCommit} title="Old name" />);

fireEvent.doubleClick(screen.getByRole("button", { name: "Old name" }));
const input = screen.getByRole("textbox", { name: "Thread name" });
fireEvent.change(input, { target: { value: "New name" } });
fireEvent.keyDown(input, { key: "Enter" });
fireEvent.blur(input);

expect(onCommit).toHaveBeenCalledTimes(1);
expect(onCommit).toHaveBeenCalledWith("New name");
});

it("does not commit when the draft is empty", () => {
const onCommit = vi.fn();
render(<InlineTitleHarness onCommit={onCommit} title="Old name" />);

fireEvent.doubleClick(screen.getByRole("button", { name: "Old name" }));
const input = screen.getByRole("textbox", { name: "Thread name" });
fireEvent.change(input, { target: { value: " " } });
fireEvent.blur(input);

expect(onCommit).not.toHaveBeenCalled();
expect(screen.getByRole("button", { name: "Old name" })).not.toBeNull();
});

it("cancels an open edit when the thread identity changes", () => {
const firstCommit = vi.fn();
const secondCommit = vi.fn();
const { rerender } = render(
<InlineTitleHarness onCommit={firstCommit} title="Old name" />,
);

fireEvent.doubleClick(screen.getByRole("button", { name: "Old name" }));
fireEvent.change(screen.getByRole("textbox", { name: "Thread name" }), {
target: { value: "Draft name" },
});

rerender(
<InlineTitleHarness
onCommit={secondCommit}
resetKey="thr_other"
title="Other thread"
/>,
);

expect(screen.queryByRole("textbox", { name: "Thread name" })).toBeNull();
expect(screen.getByRole("button", { name: "Other thread" })).not.toBeNull();
expect(firstCommit).not.toHaveBeenCalled();
expect(secondCommit).not.toHaveBeenCalled();
});
});
Loading
Loading