fix: preserve plan comment drafts on foreground refresh (#3652)

This commit is contained in:
José Almeida
2026-09-14 11:13:48 +02:00
committed by GitHub
parent ba960f9732
commit 9475abe480
12 changed files with 949 additions and 13 deletions
@@ -46,6 +46,7 @@ vi.mock("./plan-slash-menu", () => ({ PlanSlashMenu: () => null }));
import { TipTapPlanEditor } from "./tiptap-plan-editor";
const EDITOR_SELECTOR = ".ProseMirror";
const TABLE_MARKDOWN = ["| Left | Right |", "| --- | --- |", "| One | Two |"].join("\n");
function hasPluginKeyPrefix(editor: Editor, prefix: string): boolean {
@@ -226,7 +227,7 @@ describe("TipTapPlanEditor mobile formatting clearance", () => {
act(() => planBubble.onMobileVisibilityChange?.(true, 300, true));
const editorContent = editorScrollContainer?.querySelector<HTMLElement>(".ProseMirror");
const editorContent = editorScrollContainer?.querySelector<HTMLElement>(EDITOR_SELECTOR);
expect(editorContent?.style.getPropertyValue("--plan-toolbar-clearance")).toBe(
"max(48px, calc(348px - 3.25rem - env(safe-area-inset-bottom, 0px)))",
);
@@ -341,3 +342,28 @@ describe("TipTapPlanEditor comment projection", () => {
expect(onCommentDeleted).toHaveBeenCalledTimes(1);
});
});
// @covers AC-TASKS-PLAN-COMMENTS-001.9
it("toggles read-only without replacing the editor or publishing content", async () => {
const onChange = vi.fn();
const onReady = vi.fn();
const props = { taskId: "task-1", value: TABLE_MARKDOWN, onChange, onEditorReady: onReady };
const view = render(<TipTapPlanEditor {...props} />);
await waitFor(() => expect(onReady).toHaveBeenCalledTimes(1));
const editor = onReady.mock.calls[0][0] as Editor;
const input = view.container.querySelector(EDITOR_SELECTOR);
expect(editor.isEditable).toBe(true);
view.rerender(<TipTapPlanEditor {...props} readOnly />);
expect(editor.isEditable).toBe(false);
expect(view.container.querySelector(EDITOR_SELECTOR)).toBe(input);
expect(input?.getAttribute("contenteditable")).toBe("false");
view.rerender(<TipTapPlanEditor {...props} readOnly={false} />);
expect(editor.isEditable).toBe(true);
expect(view.container.querySelector(EDITOR_SELECTOR)).toBe(input);
expect(input?.getAttribute("contenteditable")).toBe("true");
expect(onReady).toHaveBeenCalledTimes(1);
expect(onChange).not.toHaveBeenCalled();
expect(getMarkdown(editor)).toContain("Left");
});
@@ -69,6 +69,7 @@ type TipTapPlanEditorProps = {
value: string;
onChange: (value: string) => void;
placeholder?: string;
readOnly?: boolean;
onSelectionChange?: (selection: TextSelection | null) => void;
comments?: CommentForEditor[];
onCommentClick?: (id: string, position: { x: number; y: number }) => void;
@@ -395,6 +396,7 @@ function usePlanEditor(props: TipTapPlanEditorProps): PlanEditorState {
const editor = useEditor(
{
immediatelyRender: false,
editable: !props.readOnly,
extensions,
content: value,
editorProps: {
@@ -424,6 +426,10 @@ function usePlanEditor(props: TipTapPlanEditorProps): PlanEditorState {
[tableResizeEnabled],
);
useLayoutEffect(() => {
editor?.setEditable(!props.readOnly, false);
}, [editor, props.readOnly]);
useEffect(() => {
editorRef.current = editor;
}, [editor]);
@@ -496,7 +502,7 @@ export function TipTapPlanEditor(props: TipTapPlanEditorProps) {
data-testid="plan-editor-scroll-container"
className={cn("h-full min-h-0 overflow-y-auto overscroll-contain")}
/>
{editor && isReady && (
{editor && isReady && !props.readOnly && (
<>
<PlanBubbleMenu
editor={editor}
@@ -508,11 +514,13 @@ export function TipTapPlanEditor(props: TipTapPlanEditorProps) {
<PlanDragHandle editor={editor} />
</>
)}
<PlanSlashMenu
menuState={slash.menuState}
selectedIndex={slash.selectedIndex}
setSelectedIndex={slash.setSelectedIndex}
/>
{!props.readOnly && (
<PlanSlashMenu
menuState={slash.menuState}
selectedIndex={slash.selectedIndex}
setSelectedIndex={slash.setSelectedIndex}
/>
)}
{!isReady && (
<div className="absolute inset-0 flex items-center justify-center text-muted-foreground text-sm bg-background/80">
{t("editors:loadingEditor")}
@@ -0,0 +1,230 @@
import { act, cleanup, fireEvent, render, screen } from "@testing-library/react";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import type { StoreApi } from "zustand";
import { StateProvider, useAppStoreApi } from "@/components/state-provider";
import type { AppState } from "@/lib/state/store";
import type { TaskPlan, TaskPlanCommentSnapshot } from "@/lib/types/http";
import type { TextSelection } from "@/components/editors/tiptap/tiptap-plan-editor";
const mocks = vi.hoisted(() => ({
touch: false,
getTaskPlan: vi.fn(),
getTaskPlanComments: vi.fn(),
createTaskPlanComment: vi.fn(),
updateTaskPlanComment: vi.fn(),
deleteTaskPlanComment: vi.fn(),
}));
vi.mock("@/lib/api/domains/plan-api", () => ({
getTaskPlan: mocks.getTaskPlan,
listPlanRevisions: async () => [],
}));
vi.mock("@/lib/api/domains/plan-comment-api", () => mocks);
vi.mock("@/hooks/use-compact-task-chrome", () => ({ useTouchDrawer: () => mocks.touch }));
vi.mock("./task-plan-panel-header", () => ({ PlanPanelHeader: () => null }));
vi.mock("@/hooks/domains/comments/use-plan-comment-migration", () => ({
usePlanCommentMigration: () => ({ needsAttention: false }),
}));
vi.mock("@/lib/routing/client-dynamic", () => ({
default:
() =>
({
onSelectionChange,
onCommentClick,
}: {
onSelectionChange: (selection: TextSelection) => void;
onCommentClick: (id: string, position: { x: number; y: number }) => void;
}) => (
<>
<button
onClick={() =>
onSelectionChange({ text: "Plan step", from: 1, to: 10, position: { x: 20, y: 20 } })
}
>
Select plan text
</button>
<button onClick={() => onCommentClick("comment-1", { x: 20, y: 20 })}>
Edit saved comment
</button>
</>
),
}));
import { TaskPlanPanel } from "./task-plan-panel";
const TASK = "task-1";
const TIMESTAMP = "2026-09-13T00:00:00Z";
const DRAFT = "Unfinished feedback with\na second line";
const PLACEHOLDER = "Add your comment or instruction...";
const plan: TaskPlan = {
id: "plan-1",
task_id: TASK,
title: "Plan",
content: "Plan step",
created_by: "agent",
created_at: TIMESTAMP,
updated_at: TIMESTAMP,
};
const snapshot: TaskPlanCommentSnapshot = {
task_id: TASK,
plan_id: plan.id,
revision: 1,
comments: [
{
id: "comment-1",
task_id: TASK,
plan_id: plan.id,
body: "Saved feedback",
selected_text: "Plan step",
anchor_from: 1,
anchor_to: 10,
version: 1,
created_at: TIMESTAMP,
updated_at: TIMESTAMP,
},
],
};
let store: StoreApi<AppState>;
function CaptureStore() {
store = useAppStoreApi();
return null;
}
function mountPanel(taskId = TASK) {
return render(
<StateProvider>
<CaptureStore />
<TaskPlanPanel taskId={taskId} />
</StateProvider>,
);
}
function openDraft(editing = false) {
fireEvent.click(
screen.getByRole("button", { name: editing ? "Edit saved comment" : "Select plan text" }),
);
const input = screen.getByPlaceholderText(PLACEHOLDER) as HTMLTextAreaElement;
fireEvent.change(input, { target: { value: DRAFT } });
return input;
}
beforeEach(() => {
vi.clearAllMocks();
mocks.getTaskPlan.mockResolvedValue(plan);
mocks.getTaskPlanComments.mockResolvedValue(snapshot);
});
afterEach(() => {
cleanup();
mocks.touch = false;
vi.restoreAllMocks();
});
// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10
for (const touch of [false, true]) {
describe(touch ? "phone comment refresh" : "desktop comment refresh", () => {
for (const editing of [false, true]) {
for (const failure of [false, true]) {
it(`preserves ${editing ? "unsaved edits" : "a new comment"} through ${failure ? "failed" : "successful"} foreground refresh`, async () => {
mocks.touch = touch;
mountPanel();
await act(async () => {
store.getState().setTaskPlan(TASK, plan);
store.getState().setTaskPlanComments(TASK, snapshot);
store.getState().setConnectionStatus("connected");
});
const input = openDraft(editing);
let resolve!: (value: TaskPlan) => void;
let reject!: (error: Error) => void;
mocks.getTaskPlan.mockImplementationOnce(
() =>
new Promise<TaskPlan>((yes, no) => {
resolve = yes;
reject = no;
}),
);
const before = mocks.getTaskPlan.mock.calls.length;
vi.spyOn(Date, "now").mockReturnValue(Date.now() + 1000);
await act(async () => {
window.dispatchEvent(new Event("focus"));
});
expect(mocks.getTaskPlan).toHaveBeenCalledTimes(before + 1);
expect(store.getState().taskPlans.loadingByTaskId[TASK]).toBe(true);
try {
expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBe(input);
expect(input.value).toBe(DRAFT);
} finally {
await act(async () => {
if (failure) reject(new Error("offline"));
else resolve(plan);
});
}
expect(screen.getByPlaceholderText(PLACEHOLDER)).toBe(input);
expect(input.value).toBe(DRAFT);
expect(screen.getByText(/Plan step/)).toBeTruthy();
expect(mocks.createTaskPlanComment).not.toHaveBeenCalled();
expect(mocks.updateTaskPlanComment).not.toHaveBeenCalled();
const updated = {
...snapshot,
revision: 2,
comments: [{ ...snapshot.comments[0], body: DRAFT, version: 2 }],
};
mocks.createTaskPlanComment.mockImplementation(async ({ id }: { id: string }) => ({
...updated,
comments: [...snapshot.comments, { ...updated.comments[0], id }],
}));
mocks.updateTaskPlanComment.mockResolvedValue(updated);
await act(async () => {
fireEvent.click(screen.getByRole("button", { name: editing ? "Update" : "Add" }));
});
const mutation = editing ? mocks.updateTaskPlanComment : mocks.createTaskPlanComment;
expect(mutation).toHaveBeenCalledTimes(1);
expect(mutation).toHaveBeenCalledWith(
expect.objectContaining({ body: DRAFT, taskId: TASK, planId: plan.id }),
);
if (!editing) {
expect(mutation).toHaveBeenCalledWith(
expect.objectContaining({ selectedText: "Plan step", anchorFrom: 1, anchorTo: 10 }),
);
}
expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull();
});
}
}
});
}
// @covers AC-TASKS-PLAN-COMMENTS-001.11
it("shows initial loading without a current plan", () => {
mountPanel();
act(() => store.getState().setTaskPlanLoading(TASK, true));
expect(screen.queryByTestId("plan-panel")).toBeNull();
expect(screen.getByText("Loading plan...")).toBeTruthy();
});
for (const replacement of [null, { ...plan, id: "plan-2" }]) {
it(`clears the draft on confirmed plan ${replacement ? "replacement" : "deletion"}`, () => {
mountPanel();
act(() => {
store.getState().setTaskPlan(TASK, plan);
});
openDraft();
act(() => {
store.getState().setTaskPlan(TASK, replacement);
});
expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull();
});
}
it("does not carry a draft to a different task that is loading", () => {
const view = mountPanel();
act(() => {
store.getState().setTaskPlan(TASK, plan);
});
openDraft();
act(() => store.getState().setTaskPlanLoading("task-2", true));
view.rerender(
<StateProvider>
<CaptureStore />
<TaskPlanPanel taskId="task-2" />
</StateProvider>,
);
expect(screen.queryByPlaceholderText(PLACEHOLDER)).toBeNull();
expect(screen.queryByTestId("plan-panel")).toBeNull();
});
+2 -1
View File
@@ -187,7 +187,7 @@ export const TaskPlanPanel = memo(function TaskPlanPanel({
title: state.plan?.title,
});
if (state.isLoading) {
if (state.isLoading && !state.plan) {
return (
<div className="flex h-full items-center justify-center text-muted-foreground">
<IconLoader2 className="h-5 w-5 animate-spin mr-2" />
@@ -260,6 +260,7 @@ function PlanPanelContent({
key={`${taskId}-${state.editorKey}`}
taskId={taskId}
value={state.draftContent}
readOnly={state.isLoading}
onChange={state.setDraftContent}
placeholder={t("task:startTypingYourPlan")}
mobileBottomOffset={mobileBottomOffset}
@@ -129,10 +129,13 @@ test("dense and sparse workflows size independently and keep the final task reac
await testPage.setViewportSize({ width: 1440, height: 900 });
await withHeightWorkflows(apiClient, seedData, async (second) => {
const denseTaskCount = 439;
await seedLargeColumnTasks(apiClient, seedData, "Dense height", denseTaskCount);
const { tasks } = await apiClient.listTasks(seedData.workspaceId);
const finalTask = tasks.find((task) => task.title === `Dense height ${denseTaskCount}`);
if (!finalTask) throw new Error("final dense task was not created");
await seedLargeColumnTasks(apiClient, seedData, "Dense height", denseTaskCount - 1);
// Create the tail after concurrent seeding so its assigned position is last.
const finalTask = await apiClient.createTask(
seedData.workspaceId,
`Dense height ${denseTaskCount}`,
{ workflow_id: seedData.workflowId, workflow_step_id: seedData.startStepId },
);
const kanban = new KanbanPage(testPage);
await kanban.goto();
const dense = kanban.columnByStepId(seedData.startStepId);
@@ -1,3 +1,4 @@
import { assertCommentSurvivesForeground } from "./plan-comment-foreground-helpers";
// Filename starts with "mobile-" so this runs under the mobile-chrome project.
import { test, expect } from "../../fixtures/test-base";
import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions";
@@ -230,3 +231,24 @@ test.describe("mobile: task-owned plan comments", () => {
).toBe(false);
});
});
// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10
for (const editing of [false, true]) {
for (const failedRead of [false, true]) {
test(`foreground refresh preserves ${editing ? "edited" : "new"} comment on ${failedRead ? "failure" : "success"}`, async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(120_000);
await assertCommentSurvivesForeground({
testPage,
apiClient,
seedData,
mobile: true,
editing,
failedRead,
});
});
}
}
@@ -0,0 +1,170 @@
import type { Page } from "@playwright/test";
import { test, expect, type SeedData } from "../../fixtures/test-base";
import type { ApiClient } from "../../helpers/api-client";
import { planScript } from "../../helpers/seed-session-messages";
import { assertNoDocumentHorizontalOverflow } from "../../helpers/layout-assertions";
import { SessionPage } from "../../pages/session-page";
type Frame = { id?: string; type?: string; action?: string; payload?: { task_id?: string } };
type Options = {
testPage: Page;
apiClient: ApiClient;
seedData: SeedData;
mobile: boolean;
editing: boolean;
failedRead: boolean;
};
const DRAFT = "Keep this unfinished feedback\nincluding the second line";
async function holdForegroundRead(page: Page, taskId: string, fail: boolean) {
let armed = false;
let release: (() => void) | undefined;
let mutations = 0;
await page.routeWebSocket(/\/ws$/, (ws) => {
const server = ws.connectToServer();
server.onMessage((message) => ws.send(message));
ws.onMessage((message) => {
if (typeof message !== "string") return server.send(message);
for (const part of message.split("\n").filter(Boolean)) {
const frame = JSON.parse(part) as Frame;
const ownsRequest = frame.type === "request" && frame.payload?.task_id === taskId;
if (
armed &&
ownsRequest &&
/task\.plan\.comments\.(create|update)/.test(frame.action ?? "")
)
mutations++;
if (armed && !release && ownsRequest && frame.action === "task.plan.get") {
release = () => {
armed = false;
if (fail)
ws.send(
JSON.stringify({
type: "error",
id: frame.id,
action: frame.action,
payload: { code: "INTERNAL_ERROR", message: "Injected foreground read failure" },
}),
);
else server.send(part);
};
} else server.send(part);
}
});
});
return {
arm: () => {
armed = true;
},
held: () => Boolean(release),
mutations: () => mutations,
release: () => release?.(),
};
}
async function openComment(page: Page, session: SessionPage, mobile: boolean) {
const editor = session.planPanel.locator(".ProseMirror:visible");
await expect(editor).toBeVisible();
await editor.focus();
const modifier = process.platform === "darwin" ? "Meta" : "Control";
await page.keyboard.press(`${modifier}+a`);
if (mobile) await page.getByTestId("plan-formatting-action-comment").tap();
else await page.keyboard.press(`${modifier}+Shift+c`);
}
export async function assertCommentSurvivesForeground(options: Options) {
const { testPage: page, apiClient, seedData, mobile, editing, failedRead } = options;
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Foreground comment draft",
seedData.agentProfileId,
{
description: planScript("## Review plan\n\nPreserve this plan comment"),
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await expect.poll(() => apiClient.getTaskPlan(task.id), { timeout: 30_000 }).not.toBeNull();
const control = await holdForegroundRead(page, task.id, failedRead);
await page.goto(`/t/${task.id}`);
const session = new SessionPage(page);
await session.waitForLoad();
await session.waitForChatIdle();
await session.togglePlanMode();
if (mobile)
await page.getByRole("navigation").getByRole("button", { name: "Plan", exact: true }).tap();
await openComment(page, session, mobile);
const input = page.locator('textarea[placeholder="Add your comment or instruction..."]:visible');
if (editing) {
await input.fill("Original saved feedback");
await page.getByRole("button", { name: "Add", exact: true }).click();
const badge = session.planPanel.locator(".comment-badge[data-comment-id]");
await expect(badge).toHaveCount(1);
const highlight = session.planPanel
.locator(".comment-highlight")
.filter({ hasText: "Preserve this plan comment" });
if (mobile) await highlight.tap();
else await highlight.click();
await expect(input).toHaveValue("Original saved feedback");
}
await input.fill(DRAFT);
const originalInput = await input.elementHandle();
control.arm();
if (!mobile) {
const otherTab = await page.context().newPage();
try {
await otherTab.bringToFront();
await page.bringToFront();
} finally {
await otherTab.close();
}
}
// Headless browsers may not emit OS focus events for bringToFront.
// Dispatch the foreground event too; the production hook coalesces duplicates.
await page.evaluate(() => window.dispatchEvent(new Event("focus")));
await expect.poll(control.held).toBe(true);
try {
await expect(session.planPanel.locator(".ProseMirror")).toHaveAttribute(
"contenteditable",
"false",
);
await expect(input).toBeEditable();
await expect(input).toHaveValue(DRAFT);
expect(await originalInput!.evaluate((element) => element.isConnected)).toBe(true);
expect(control.mutations()).toBe(0);
} finally {
control.release();
}
await expect
.poll(() =>
page.evaluate((taskId) => {
const win = window as Window & {
__KANDEV_E2E_STORE__?: {
getState: () => { taskPlans: { loadingByTaskId: Record<string, boolean> } };
};
};
return win.__KANDEV_E2E_STORE__?.getState().taskPlans.loadingByTaskId[taskId];
}, task.id),
)
.toBe(false);
await expect(input).toHaveValue(DRAFT);
expect(await originalInput!.evaluate((element) => element.isConnected)).toBe(true);
await expect(session.planPanel.locator(".ProseMirror")).toHaveAttribute(
"contenteditable",
"true",
);
if (mobile) await assertNoDocumentHorizontalOverflow(page, "foreground comment draft");
await page.screenshot({ path: test.info().outputPath("foreground-comment.png") });
await page.getByRole("button", { name: editing ? "Update" : "Add", exact: true }).click();
await expect
.poll(async () => {
const snapshot = await apiClient.wsRequest<{ comments: Array<{ body: string }> }>(
"task.plan.comments.list",
{ task_id: task.id },
);
return snapshot.comments.map((comment) => comment.body);
})
.toEqual([DRAFT]);
await originalInput?.dispose();
}
@@ -1,3 +1,4 @@
import { assertCommentSurvivesForeground } from "./plan-comment-foreground-helpers";
import type { Page } from "@playwright/test";
import { test, expect } from "../../fixtures/test-base";
import { planScript } from "../../helpers/seed-session-messages";
@@ -191,3 +192,24 @@ test.describe("task-owned plan comments", () => {
});
});
});
// @covers AC-TASKS-PLAN-COMMENTS-001.9, AC-TASKS-PLAN-COMMENTS-001.10
for (const editing of [false, true]) {
for (const failedRead of [false, true]) {
test(`foreground refresh preserves ${editing ? "edited" : "new"} comment on ${failedRead ? "failure" : "success"}`, async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(120_000);
await assertCommentSurvivesForeground({
testPage,
apiClient,
seedData,
mobile: false,
editing,
failedRead,
});
});
}
}